Split SequenceKeyGenerator into 3 implementations handling one of the styles before, during and after

Details

Type: Sub-task
Status: Closed
Priority: Minor
Resolution: Fixed
Affects Version/s: 1.3
Fix Version/s: 1.3.1
Component/s: JDO queries
Labels:
None

Number of attachments :
2

Description

Move decision which style and therefore which implementation to use to SequenceKeyGeneratorFactory.

Options
- Sort By Name
- Sort By Date
- Ascending
- Descending
- Download All

Attachments

patch-C2721-20090722.txt

22/Jul/09 10:59 AM

63 kB

AHMAD HASSAN
patch-C2721-20090725.txt

25/Jul/09 6:10 PM

67 kB

Ralf Joachim

Activity

Ascending order - Click to sort in descending order

Hide

Permalink

AHMAD HASSAN added a comment - 21/Jul/09 12:40 PM

Hi Ralf,

I am in the process of returning appropriate SKG instance from SKG factory based on three styles we have [Before, After, During]. While looking at the SKG constructor, the code looks bit weird as far as style allocation is concerned. The code looks like the following

        
        boolean returning = "true".equals(params.getProperty("returning"));
        _triggerPresent = "true".equals(params.getProperty("trigger", "false"));

        if (returning) {
            if (!_factoryName.equals(OracleFactory.FACTORY_NAME)
                    && !_factoryName.equals(PostgreSQLFactory.FACTORY_NAME)) {
                throw new MappingException(Messages.format("mapping.keyGenParamNotCompat",
                        "returning=\"true\"", getClass().getName(), _factoryName));
            }
        }

        _style = _factoryName.equals(PostgreSQLFactory.FACTORY_NAME)
                || _factoryName.equals(InterbaseFactory.FACTORY_NAME)
                || _factoryName.equals(DB2Factory.FACTORY_NAME)
                ? BEFORE_INSERT : (returning ? DURING_INSERT : AFTER_INSERT);
        if (_triggerPresent && !returning) {
            _style = AFTER_INSERT;
        }
        if (_triggerPresent && (_style == BEFORE_INSERT)) {
            throw new MappingException(Messages.format("mapping.keyGenParamNotCompat",
                    "trigger=\"true\"", getClass().getName(), _factoryName));
        }

The styling decision depends on the factory name, trigger presence and returning value. The following 'if' condition in the above is quite tricky and very dirty hack in to the code. I don't know how to handle this scenerio in our intialization.

      if (_triggerPresent && !returning) {
            _style = AFTER_INSERT;
        }

One more thing I want to ask is, Do we provide factory methods in persistance factory to see whether it support 'before', 'after' or 'during' style or we handle it in the way as it is handling currently in the form of 'if' statements.

Also I was thinking to implement the following two additional methods for SKG as you did for IKG

 boolean isKeyGeneratorSequenceTypeSupported(int type);
 boolean isKeyGeneratorIdentitySupported();

Does this makes sense? or we keep the current methods present in SKG as supportsSQLType and checkSupportedFactory(factory);

Regards, Ahmad

Show

AHMAD HASSAN added a comment - 21/Jul/09 12:40 PM Hi Ralf, I am in the process of returning appropriate SKG instance from SKG factory based on three styles we have [Before, After, During] . While looking at the SKG constructor, the code looks bit weird as far as style allocation is concerned. The code looks like the following boolean returning = " true " .equals(params.getProperty( "returning" )); _triggerPresent = " true " .equals(params.getProperty( "trigger" , " false " )); if (returning) { if (!_factoryName.equals(OracleFactory.FACTORY_NAME) && !_factoryName.equals(PostgreSQLFactory.FACTORY_NAME)) { throw new MappingException(Messages.format( "mapping.keyGenParamNotCompat" , "returning=\" true \"", getClass().getName(), _factoryName)); } } _style = _factoryName.equals(PostgreSQLFactory.FACTORY_NAME) || _factoryName.equals(InterbaseFactory.FACTORY_NAME) || _factoryName.equals(DB2Factory.FACTORY_NAME) ? BEFORE_INSERT : (returning ? DURING_INSERT : AFTER_INSERT); if (_triggerPresent && !returning) { _style = AFTER_INSERT; } if (_triggerPresent && (_style == BEFORE_INSERT)) { throw new MappingException(Messages.format( "mapping.keyGenParamNotCompat" , "trigger=\" true \"", getClass().getName(), _factoryName)); } The styling decision depends on the factory name, trigger presence and returning value. The following 'if' condition in the above is quite tricky and very dirty hack in to the code. I don't know how to handle this scenerio in our intialization. if (_triggerPresent && !returning) { _style = AFTER_INSERT; } One more thing I want to ask is, Do we provide factory methods in persistance factory to see whether it support 'before', 'after' or 'during' style or we handle it in the way as it is handling currently in the form of 'if' statements. Also I was thinking to implement the following two additional methods for SKG as you did for IKG boolean isKeyGeneratorSequenceTypeSupported( int type); boolean isKeyGeneratorIdentitySupported(); Does this makes sense? or we keep the current methods present in SKG as supportsSQLType and checkSupportedFactory(factory); Regards, Ahmad

Hide

Permalink

Ralf Joachim added a comment - 21/Jul/09 2:10 PM

A method at persistence factory to decide which style to use sound reasonable. Could you please outline how such a method could look like.

 boolean isKeyGeneratorSequenceTypeSupported(int type);
 boolean isKeyGeneratorSequenceSupported();

methods make sense for sequence key generator also.

Show

Ralf Joachim added a comment - 21/Jul/09 2:10 PM A method at persistence factory to decide which style to use sound reasonable. Could you please outline how such a method could look like. boolean isKeyGeneratorSequenceTypeSupported( int type); boolean isKeyGeneratorSequenceSupported(); methods make sense for sequence key generator also.

Hide

Permalink

AHMAD HASSAN added a comment - 21/Jul/09 3:10 PM

Giving it much deeper though makes me feel that we should not move the style decision to persistence facotory because only 'before' style is dependent on the type of factory where as 'During' and 'After' are not to some extent. What we can do is, we can provide a method isBeforeStyleSupported to see whether 'Before' style is supported or not. What do you think.

As far as instance intialization in SKG factory is concerned, I wrote something like follow but I am bit confused whether initialization of 'AfterSequeneGenerator' is changing the actual logic or not. I think it does..

   //SKG Factory class

    public KeyGenerator getKeyGenerator(final PersistenceFactory factory,
            final Properties params, final int sqlType) throws MappingException {

        String factoryName = factory.getFactoryName();
        boolean returning = "true".equals(params.getProperty("returning"));
        boolean triggerPresent = "true".equals(params.getProperty("trigger", "false"));

        if (returning) {
            if (!factoryName.equals(OracleFactory.FACTORY_NAME)
                    && !factoryName.equals(PostgreSQLFactory.FACTORY_NAME)) {
                throw new MappingException(Messages.format("mapping.keyGenParamNotCompat",
                        "returning=\"true\"", getClass().getName(), factoryName));
            }
        }

        if (triggerPresent && !returning) { return new AfterSequenceKeyGenerator(factory, params, sqlType); }
        if (factoryName.equals(PostgreSQLFactory.FACTORY_NAME)
                || factoryName.equals(InterbaseFactory.FACTORY_NAME)
                || factoryName.equals(DB2Factory.FACTORY_NAME)
                ) { return new BeforeSequenceKeyGenerator(factory, params, sqlType); }
        
        if(returning) { return new DuringSequenceKeyGenerator(factory, params, sqlType);  } 
        return new AfterSequenceKeyGenerator(factory, params, sqlType);       
    }

Regards, Ahmad

Show

AHMAD HASSAN added a comment - 21/Jul/09 3:10 PM Giving it much deeper though makes me feel that we should not move the style decision to persistence facotory because only 'before' style is dependent on the type of factory where as 'During' and 'After' are not to some extent. What we can do is, we can provide a method isBeforeStyleSupported to see whether 'Before' style is supported or not. What do you think. As far as instance intialization in SKG factory is concerned, I wrote something like follow but I am bit confused whether initialization of 'AfterSequeneGenerator' is changing the actual logic or not. I think it does.. //SKG Factory class public KeyGenerator getKeyGenerator( final PersistenceFactory factory, final Properties params, final int sqlType) throws MappingException { String factoryName = factory.getFactoryName(); boolean returning = " true " .equals(params.getProperty( "returning" )); boolean triggerPresent = " true " .equals(params.getProperty( "trigger" , " false " )); if (returning) { if (!factoryName.equals(OracleFactory.FACTORY_NAME) && !factoryName.equals(PostgreSQLFactory.FACTORY_NAME)) { throw new MappingException(Messages.format( "mapping.keyGenParamNotCompat" , "returning=\" true \"", getClass().getName(), factoryName)); } } if (triggerPresent && !returning) { return new AfterSequenceKeyGenerator(factory, params, sqlType); } if (factoryName.equals(PostgreSQLFactory.FACTORY_NAME) || factoryName.equals(InterbaseFactory.FACTORY_NAME) || factoryName.equals(DB2Factory.FACTORY_NAME) ) { return new BeforeSequenceKeyGenerator(factory, params, sqlType); } if (returning) { return new DuringSequenceKeyGenerator(factory, params, sqlType); } return new AfterSequenceKeyGenerator(factory, params, sqlType); } Regards, Ahmad

Hide

Permalink

AHMAD HASSAN added a comment - 21/Jul/09 3:20 PM

In fact the better form of above code is

//SKG Factory class

    public KeyGenerator getKeyGenerator(final PersistenceFactory factory,
            final Properties params, final int sqlType) throws MappingException {

        String factoryName = factory.getFactoryName();
        boolean returning = "true".equals(params.getProperty("returning"));
        boolean triggerPresent = "true".equals(params.getProperty("trigger", "false"));

        if (triggerPresent && !returning) { return new AfterSequenceKeyGenerator(factory, params, sqlType); }
        if (factoryName.equals(PostgreSQLFactory.FACTORY_NAME)
                || factoryName.equals(InterbaseFactory.FACTORY_NAME)
                || factoryName.equals(DB2Factory.FACTORY_NAME)
                ) { return new BeforeSequenceKeyGenerator(factory, params, sqlType); }
        
        if(returning) { return new DuringSequenceKeyGenerator(factory, params, sqlType);  } 
        return new AfterSequenceKeyGenerator(factory, params, sqlType);       
    }

Show

AHMAD HASSAN added a comment - 21/Jul/09 3:20 PM In fact the better form of above code is //SKG Factory class public KeyGenerator getKeyGenerator( final PersistenceFactory factory, final Properties params, final int sqlType) throws MappingException { String factoryName = factory.getFactoryName(); boolean returning = " true " .equals(params.getProperty( "returning" )); boolean triggerPresent = " true " .equals(params.getProperty( "trigger" , " false " )); if (triggerPresent && !returning) { return new AfterSequenceKeyGenerator(factory, params, sqlType); } if (factoryName.equals(PostgreSQLFactory.FACTORY_NAME) || factoryName.equals(InterbaseFactory.FACTORY_NAME) || factoryName.equals(DB2Factory.FACTORY_NAME) ) { return new BeforeSequenceKeyGenerator(factory, params, sqlType); } if (returning) { return new DuringSequenceKeyGenerator(factory, params, sqlType); } return new AfterSequenceKeyGenerator(factory, params, sqlType); }

Hide

Permalink

AHMAD HASSAN added a comment - 22/Jul/09 10:59 AM

This is pretty inital draft patch which includes the splitting of SKG into 3 implementations.

AfterSequenceKeyGenerator
BeforeSequenceKeyGenerator
DuringSequenceKeyGenerator

It involves the changes to SKG factory which makes the styling decision and which SKG to initialize. I am not much confident at this part though because this was quite tricky.

Additonally I added two more method to Persistance factory to be used by SKG.

    boolean isKeyGeneratorSequenceSupported();
    
    boolean isKeyGeneratorSequenceTypeSupported(int type);

I didn't add comments to already un commented code because this requires further refactory i.e. moving of database engine specific code to factory classes.

As a next step I think we should move that engine specific code to the factory classes. What do you advice?

Regards, Ahmad

Show

AHMAD HASSAN added a comment - 22/Jul/09 10:59 AM This is pretty inital draft patch which includes the splitting of SKG into 3 implementations. AfterSequenceKeyGenerator BeforeSequenceKeyGenerator DuringSequenceKeyGenerator It involves the changes to SKG factory which makes the styling decision and which SKG to initialize. I am not much confident at this part though because this was quite tricky. Additonally I added two more method to Persistance factory to be used by SKG. boolean isKeyGeneratorSequenceSupported(); boolean isKeyGeneratorSequenceTypeSupported( int type); I didn't add comments to already un commented code because this requires further refactory i.e. moving of database engine specific code to factory classes. As a next step I think we should move that engine specific code to the factory classes. What do you advice? Regards, Ahmad

Hide

Permalink

Ralf Joachim added a comment - 25/Jul/09 6:10 PM

Final patch. Will commit after current build problems got resolved.

Show

Ralf Joachim added a comment - 25/Jul/09 6:10 PM Final patch. Will commit after current build problems got resolved.

People

Assignee:

AHMAD HASSAN

Reporter:

Ralf Joachim

Vote (0)

Watch (0)

Dates

Created:

16/May/09 8:54 AM

Updated:

30/Dec/09 4:21 AM

Resolved:

26/Jul/09 2:33 PM