Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: JDO 3 (3.0)
    • Component/s: api, specification, tck
    • Labels:
      None

      Description

      We can specify MetaData via XML or annotations. The only way missing is via an API. I propose mirroring the XML structure with interfaces of the form

      public interface MetaData

      { addExtension(String key, String value); removeExtension(String key, String value); ... }

      public interface FileMetaData

      { addPackage(PackageMetaData pmd); ... }

      public interface PackageMetaData

      { addClass(ClassMetaData cmd) ... }

      public interface ClassMetaData

      { addField(FieldMetaData fmd) ... }

      public interface FieldMetaData

      { setInheritance(InheritanceMetaData inhmd) ... }

      and so on.

      We would then require a method on the PMF to register the metadata.

      If there are no objections to such a feature I'll propose a patch to try to provide all current JDO2 capabilities.

      1. type-2.patch
        5 kB
        Matthew T. Adams
      2. type.patch
        14 kB
        Matthew T. Adams
      3. lower-case-d-in-metadata.patch
        2 kB
        Matthew T. Adams
      4. jdometadata-6.patch
        2 kB
        Andy Jefferson
      5. BooleanProperties.patch
        1 kB
        Michael Bouschen

        Activity

        Hide
        Andy Jefferson added a comment -

        Work-in-progress patch to provide javax.jdo.metadata for use in having an API for metadata.
        Some classes still need completion to match the DTD/XSD.

        Maybe FileMetaData should be called JDOMetaData?

        I've included copies of some enums under javax.jdo.annotations. Maybe they should only be under javax.jdo.metadata ? but what about backwards compatibility?

        Interface with PMF could be as simple as
        pmf.registerMetaData(FileMetaData md);

        or we could provide access to current metadata too, so
        pmf.getMetaData(String className);

        Show
        Andy Jefferson added a comment - Work-in-progress patch to provide javax.jdo.metadata for use in having an API for metadata. Some classes still need completion to match the DTD/XSD. Maybe FileMetaData should be called JDOMetaData? I've included copies of some enums under javax.jdo.annotations. Maybe they should only be under javax.jdo.metadata ? but what about backwards compatibility? Interface with PMF could be as simple as pmf.registerMetaData(FileMetaData md); or we could provide access to current metadata too, so pmf.getMetaData(String className);
        Hide
        Andy Jefferson added a comment -

        Updated patch for basic MetaData bean-style components. Maybe some attributes not yet present - please comment.
        Also requires a MetaDataFactory for creating the implementation of each type, so we could have
        pmf.getMetaDataManager()
        which returns an implementation of "javax.jdo.metadata.MetaDataManager" (to be defined) and which has a series of factory methods for creating the implementations of the metadata components. Also has a method registerMetaData(...) allowing users to hook in their dyanmically created metadata.

        Show
        Andy Jefferson added a comment - Updated patch for basic MetaData bean-style components. Maybe some attributes not yet present - please comment. Also requires a MetaDataFactory for creating the implementation of each type, so we could have pmf.getMetaDataManager() which returns an implementation of "javax.jdo.metadata.MetaDataManager" (to be defined) and which has a series of factory methods for creating the implementations of the metadata components. Also has a method registerMetaData(...) allowing users to hook in their dyanmically created metadata.
        Hide
        Andy Jefferson added a comment -

        Further update with simple factory interface for generating metadata components. Alternative approach would be to single method on MetaDataManager newFileMetaData(), and then have a method on each parent component to create the subcomponents.

        Show
        Andy Jefferson added a comment - Further update with simple factory interface for generating metadata components. Alternative approach would be to single method on MetaDataManager newFileMetaData(), and then have a method on each parent component to create the subcomponents.
        Hide
        Andy Jefferson added a comment -

        To demonstrate a typical use-case for this API, have a look at this :-
        http://www.jpox.org/servlet/wiki/pages/viewpage.action?pageId=6619188
        This demonstrates the dynamic generation of a class in memory. We then create MetaData for it (DataNucleus API in this case). We then enhance the in-memory class. We finally use the in-memory enhanced class (and metadata) at runtime.

        JDO-591 will standardise the enhancer API, and JDO-615 will standardise the MetaData API, hence going a long way towards providing the full process without touching a particular vendors classes.

        Show
        Andy Jefferson added a comment - To demonstrate a typical use-case for this API, have a look at this :- http://www.jpox.org/servlet/wiki/pages/viewpage.action?pageId=6619188 This demonstrates the dynamic generation of a class in memory. We then create MetaData for it (DataNucleus API in this case). We then enhance the in-memory class. We finally use the in-memory enhanced class (and metadata) at runtime. JDO-591 will standardise the enhancer API, and JDO-615 will standardise the MetaData API, hence going a long way towards providing the full process without touching a particular vendors classes.
        Hide
        Craig L Russell added a comment -

        Just some high level comments for right now.

        In order to mirror the xml metadata, you would need to start with a top level metadata root, corresponding to the jdo element. You could construct this from the JDOHelper, e.g. public JDOMetadata newJDOMetadata();

        Then, JDOMetadata would have methods newPackageMetadata(String name), newQueryMetadata(String name), and newFetchPlanMetadata(String name).

        PackageMetadata would then have methods newInterfaceMetadata, newClassMetadata, and newSequenceMetadata.

        If you use method chaining instead of constructors, then you could change this example to be more self-describing:

        ClassMetaData cmd = dmdf.newClassObject(pmd,"Client", IdentityType.DATASTORE.toString(), null,
        Boolean.TRUE.toString(), Boolean.TRUE.toString(), Boolean.FALSE.toString(),
        ClassPersistenceModifier.PERSISTENCE_CAPABLE.toString(), null, null, null, "CLIENT", null);

        pkgMetadata.newClassMetadata("Client")
        .setIdentityType(IdentityType.DATASTORE)
        .setPersistenceCapableSuperclass("Person");

        Using the newClassMetadata construction would enforce the rule that a class can be a member of only one package, so you can neither forget to assign the class to a package nor add the class to more than one package.

        If you want to enforce setting the required metadata, the newXXX could do this, e.g. for sequence metadata, the name and strategy are required:
        packageMetadata.newSequence("SQ1244", SequenceStrategy.CONTIGUOUS);

        But most metadata is optional with reasonable defaults.

        For boolean properties, you might use Boolean as parameters/return values instead. Java 5 will automatically convert the setXXX parameters for you, but the getXXX will return a tristate value which is needed to distinguish between the user setting a value and not. A boolean return type doesn't allow this.

        Finally, note that in the javax.jdo.annotations package we already have enums for the metadata. So there's no need to repeat them. The only awkward thing is that they would be referenced from the metadata package. But repeating their definitions in multiple packages seems worse.

        Show
        Craig L Russell added a comment - Just some high level comments for right now. In order to mirror the xml metadata, you would need to start with a top level metadata root, corresponding to the jdo element. You could construct this from the JDOHelper, e.g. public JDOMetadata newJDOMetadata(); Then, JDOMetadata would have methods newPackageMetadata(String name), newQueryMetadata(String name), and newFetchPlanMetadata(String name). PackageMetadata would then have methods newInterfaceMetadata, newClassMetadata, and newSequenceMetadata. If you use method chaining instead of constructors, then you could change this example to be more self-describing: ClassMetaData cmd = dmdf.newClassObject(pmd,"Client", IdentityType.DATASTORE.toString(), null, Boolean.TRUE.toString(), Boolean.TRUE.toString(), Boolean.FALSE.toString(), ClassPersistenceModifier.PERSISTENCE_CAPABLE.toString(), null, null, null, "CLIENT", null); pkgMetadata.newClassMetadata("Client") .setIdentityType(IdentityType.DATASTORE) .setPersistenceCapableSuperclass("Person"); Using the newClassMetadata construction would enforce the rule that a class can be a member of only one package, so you can neither forget to assign the class to a package nor add the class to more than one package. If you want to enforce setting the required metadata, the newXXX could do this, e.g. for sequence metadata, the name and strategy are required: packageMetadata.newSequence("SQ1244", SequenceStrategy.CONTIGUOUS); But most metadata is optional with reasonable defaults. For boolean properties, you might use Boolean as parameters/return values instead. Java 5 will automatically convert the setXXX parameters for you, but the getXXX will return a tristate value which is needed to distinguish between the user setting a value and not. A boolean return type doesn't allow this. Finally, note that in the javax.jdo.annotations package we already have enums for the metadata. So there's no need to repeat them. The only awkward thing is that they would be referenced from the metadata package. But repeating their definitions in multiple packages seems worse.
        Hide
        Andy Jefferson added a comment -

        I included a top level metadata, just I called it FileMetaData, with the comment above "Maybe FileMetaData should be called JDOMetaData?"

        If you create JDOMetaData via JDOHelper then how is JDOHelper to know what the implementation of JDOMetaData is that it should create? (Could be done via META-INF/services entry).

        No problem with using "newXXXMetaData" on MetaData classes instead of having factory methods.

        Show
        Andy Jefferson added a comment - I included a top level metadata, just I called it FileMetaData, with the comment above "Maybe FileMetaData should be called JDOMetaData?" If you create JDOMetaData via JDOHelper then how is JDOHelper to know what the implementation of JDOMetaData is that it should create? (Could be done via META-INF/services entry). No problem with using "newXXXMetaData" on MetaData classes instead of having factory methods.
        Hide
        Andy Jefferson added a comment -

        Updated javax.jdo.metadata :-
        1. Remove many enums dupd from annotations, as per my previous comments on this JIRA
        2. Renamed FileMetaData to JDOMetaData
        3. Added FetchPlanMetaData
        4. Added many sub metadata components
        5. Changed add/set methods to do the creation of metadata objects as per Craig comments.
        6. Updated MetaDataManager to fit above (still has creator for JDOMetaData currently but this could move to JDOHelper at some stage)

        Show
        Andy Jefferson added a comment - Updated javax.jdo.metadata :- 1. Remove many enums dupd from annotations, as per my previous comments on this JIRA 2. Renamed FileMetaData to JDOMetaData 3. Added FetchPlanMetaData 4. Added many sub metadata components 5. Changed add/set methods to do the creation of metadata objects as per Craig comments. 6. Updated MetaDataManager to fit above (still has creator for JDOMetaData currently but this could move to JDOHelper at some stage)
        Hide
        Craig L Russell added a comment -

        > If you create JDOMetaData via JDOHelper then how is JDOHelper to know what the implementation of JDOMetaData is that it should create? (Could be done via META-INF/services entry).

        Right. So maybe the JDOMetadata factory method belongs on PersistenceManagerFactory.

        > 6. Updated MetaDataManager to fit above (still has creator for JDOMetaData currently but this could move to JDOHelper at some stage)

        If you have newXXX methods you don't need a MetadataManager at all.

        One more "big" thing. Metadata shouldn't be camel case. It's a real word, not two words that need a camel in the middle. See http://en.wikipedia.org/wiki/Metadata. So I'd really prefer lower-casing the "d". (Note how hard it is to remember the capital "s" in DataStore?)

        Show
        Craig L Russell added a comment - > If you create JDOMetaData via JDOHelper then how is JDOHelper to know what the implementation of JDOMetaData is that it should create? (Could be done via META-INF/services entry). Right. So maybe the JDOMetadata factory method belongs on PersistenceManagerFactory. > 6. Updated MetaDataManager to fit above (still has creator for JDOMetaData currently but this could move to JDOHelper at some stage) If you have newXXX methods you don't need a MetadataManager at all. One more "big" thing. Metadata shouldn't be camel case. It's a real word, not two words that need a camel in the middle. See http://en.wikipedia.org/wiki/Metadata . So I'd really prefer lower-casing the "d". (Note how hard it is to remember the capital "s" in DataStore?)
        Hide
        Andy Jefferson added a comment -

        If you create JDOMetaData via a services entry and there are more than 1 possible in the CLASSPATH how does JDOHelper know that it is picking the right one that is compatible with the JDO implementation that will be used? e.g an app using JDO impl 1 for legacy module 1, and JDO impl 2 for new module 2.
        While all implementations of XXXMetadata will implement it, it is reasonable for a JDO implementation to only accept its own implementation of the XXXMetadata interfaces to be registered.

        Regarding spelling : No comment (to be made by me on American spellings )

        Show
        Andy Jefferson added a comment - If you create JDOMetaData via a services entry and there are more than 1 possible in the CLASSPATH how does JDOHelper know that it is picking the right one that is compatible with the JDO implementation that will be used? e.g an app using JDO impl 1 for legacy module 1, and JDO impl 2 for new module 2. While all implementations of XXXMetadata will implement it, it is reasonable for a JDO implementation to only accept its own implementation of the XXXMetadata interfaces to be registered. Regarding spelling : No comment (to be made by me on American spellings )
        Hide
        Andy Jefferson added a comment -

        Maybe best to just skip the whole services thing for JDOMetaData. Just add the following to the PMF (and JDOEnhancer)

        JDOMetadata newMetadata()
        void registerMetadata(JDOMetadata)

        Show
        Andy Jefferson added a comment - Maybe best to just skip the whole services thing for JDOMetaData. Just add the following to the PMF (and JDOEnhancer) JDOMetadata newMetadata() void registerMetadata(JDOMetadata)
        Hide
        Craig L Russell added a comment -

        > Maybe best to just skip the whole services thing for JDOMetaData. Just add the following to the PMF (and JDOEnhancer)
        >
        > JDOMetadata newMetadata()
        > void registerMetadata(JDOMetadata)

        Cool.

        When adding metadata to PMF and enhancer, what override rules do we want? That is, we need to take a look at this part of Chapter 18:

        JDO 2.2 218 October 10, 2008
        When metadata information is needed for a class, and the metadata for that class has not already
        been loaded, the metadata is searched for as follows: META-INF/package.jdo, WEB-INF/pack-
        age.jdo, package.jdo, <package>/.../<package>/package.jdo, and <package>/<class>.jdo. Once
        metadata for a class has been loaded, the metadata will not be replaced in memory as long as the
        class is not garbage collected. Therefore, metadata contained higher in the search order will always
        be used instead of metadata contained lower in the search order.

        So where in this hierarchy does metadata added via calls (I assume that you can make many calls to registerMetadata) go?

        Show
        Craig L Russell added a comment - > Maybe best to just skip the whole services thing for JDOMetaData. Just add the following to the PMF (and JDOEnhancer) > > JDOMetadata newMetadata() > void registerMetadata(JDOMetadata) Cool. When adding metadata to PMF and enhancer, what override rules do we want? That is, we need to take a look at this part of Chapter 18: JDO 2.2 218 October 10, 2008 When metadata information is needed for a class, and the metadata for that class has not already been loaded, the metadata is searched for as follows: META-INF/package.jdo, WEB-INF/pack- age.jdo, package.jdo, <package>/.../<package>/package.jdo, and <package>/<class>.jdo. Once metadata for a class has been loaded, the metadata will not be replaced in memory as long as the class is not garbage collected. Therefore, metadata contained higher in the search order will always be used instead of metadata contained lower in the search order. So where in this hierarchy does metadata added via calls (I assume that you can make many calls to registerMetadata) go?
        Hide
        Andy Jefferson added a comment -

        Updated patch with namings, moved methods from MetadataManager to PersistenceManagerFactory, and add more missing relations in metadata tree.

        Re: priority of user-defined metadata.
        Why not make user-defined metadata highest priority? That way the user has a way of overriding some package.jdo in the CLASSPATH which they can treat as their fallback. Any problems with this approach that you can see?

        Other item : do we want to provide access to metadata for a class being used by the persistence process ? I'd favour something simple like
        ClassMetadata pmf.getMetadata(String classname);
        This will allow applications to access mapping info (e.g table used for a class) and utilise it in a different part of their app. Obviously it would be down to individual implementations whether they support any subsequent changes to that metadata if handed out to the user.

        Show
        Andy Jefferson added a comment - Updated patch with namings, moved methods from MetadataManager to PersistenceManagerFactory, and add more missing relations in metadata tree. Re: priority of user-defined metadata. Why not make user-defined metadata highest priority? That way the user has a way of overriding some package.jdo in the CLASSPATH which they can treat as their fallback. Any problems with this approach that you can see? Other item : do we want to provide access to metadata for a class being used by the persistence process ? I'd favour something simple like ClassMetadata pmf.getMetadata(String classname); This will allow applications to access mapping info (e.g table used for a class) and utilise it in a different part of their app. Obviously it would be down to individual implementations whether they support any subsequent changes to that metadata if handed out to the user.
        Hide
        Craig L Russell added a comment -

        > Re: priority of user-defined metadata.
        > Why not make user-defined metadata highest priority? That way the user has a way of overriding some package.jdo in the CLASSPATH which they can treat as their fallback.

        The rationale for the algorithm used today is that once metadata has been defined, there may be other metadata that depends on it (e.g. inheritance and relationships). So if you change this you are in danger of breaking things that are working for other parts of the application. I don't have a use-case for changing metadata that's already been defined. Do you?

        > Other item : do we want to provide access to metadata for a class being used by the persistence process ? I'd favour something simple like ClassMetadata pmf.getMetadata(String classname);
        > This will allow applications to access mapping info (e.g table used for a class) and utilise it in a different part of their app. Obviously it would be down to individual implementations whether they support any subsequent changes to that metadata if handed out to the user.

        There are two questions: do we support a way for an application to get the current definition of metadata? I'd say yes.

        Do we support changing metadata by modifying the metadata objects that are given to the application? Without a clear understanding of the use-case for this (see above) I'd say no thanks. The metadata instances should be immutable if given out, or at least give out a copy of what's defined, that by definition doesn't change the current copy. Seems like it's "easy enough" to copy metadata.

        Yet another more topic: security. We probably need to add a security bit to the registerMetadata and getMetadata calls. We already have the getMetadata permission but might want a setMetadata permission as well.

        Show
        Craig L Russell added a comment - > Re: priority of user-defined metadata. > Why not make user-defined metadata highest priority? That way the user has a way of overriding some package.jdo in the CLASSPATH which they can treat as their fallback. The rationale for the algorithm used today is that once metadata has been defined, there may be other metadata that depends on it (e.g. inheritance and relationships). So if you change this you are in danger of breaking things that are working for other parts of the application. I don't have a use-case for changing metadata that's already been defined. Do you? > Other item : do we want to provide access to metadata for a class being used by the persistence process ? I'd favour something simple like ClassMetadata pmf.getMetadata(String classname); > This will allow applications to access mapping info (e.g table used for a class) and utilise it in a different part of their app. Obviously it would be down to individual implementations whether they support any subsequent changes to that metadata if handed out to the user. There are two questions: do we support a way for an application to get the current definition of metadata? I'd say yes. Do we support changing metadata by modifying the metadata objects that are given to the application? Without a clear understanding of the use-case for this (see above) I'd say no thanks. The metadata instances should be immutable if given out, or at least give out a copy of what's defined, that by definition doesn't change the current copy. Seems like it's "easy enough" to copy metadata. Yet another more topic: security. We probably need to add a security bit to the registerMetadata and getMetadata calls. We already have the getMetadata permission but might want a setMetadata permission as well.
        Hide
        Craig L Russell added a comment -

        Yet more random comments on patch 5:

        InterfaceMetadata extends both Metadata and ComponentMetadata. But ComponentMetadata already extends Metadata...

        ComponentMetadata might include get/setPersistenceModifier since presumably this applies to interfaces as well. That leaves only FieldMetadata for ClassMetadata which seems correct.

        There are several cases of setXXX that take non-enum, non-primitive values. I'd like to eliminate them entirely to avoid user error (reusing metadata elements in multiple places). Can each of these cases be justified? Or can we add factory methods to the interfaces instead (e.g. newVersionMetadata instead of setVersionMetadata)? Then there's no need for setParent either.

        ComponentMetadata:
        setVersionMetadata
        setVersionMetadata
        setDatastoreIdentityMetadata
        setPrimaryKeyMetadata

        DiscriminatorMetadata:
        setIndexMetadata

        ElementMetadata:
        setEmbeddedMetadata
        setIndexMetadata
        setUniqueMetadata
        setForeignKeyMetadata

        JoinMetadata:
        setForeignKeyMetadata
        setPrimaryKeyMetadata

        KeyMetadata:
        setEmbeddedMetadata
        setIndexMetadata
        setUniqueMetadata
        setForeignKeyMetadata

        MemberMetadata:
        setArrayMetadata
        setCollectionMetadata
        setMapMetadata
        setJoinMetadata
        setEmbeddedMetadata
        setElementMetadata
        setKeyMetadata
        setValueMetadata
        setIndexMetadata
        setUniqueMetadata
        setForeignKeyMetadata

        Metadata:
        setParent

        OrderMetadata:
        setIndexMetadata

        ValueMetadata:
        setEmbeddedMetadata
        setIndexMetadata
        setUniqueMetadata
        setForeignKeyMetadata

        VersionMetadata:
        setIndexMetadata

        In SequenceMetadata, not sure if you intended this to be Class or String... None of the other metadata calls use Class.
        + * Method to set the result class name for the query
        + *
        + * @param cls Result class name
        + */
        + void setFactoryClass(Class cls);

        Show
        Craig L Russell added a comment - Yet more random comments on patch 5: InterfaceMetadata extends both Metadata and ComponentMetadata. But ComponentMetadata already extends Metadata... ComponentMetadata might include get/setPersistenceModifier since presumably this applies to interfaces as well. That leaves only FieldMetadata for ClassMetadata which seems correct. There are several cases of setXXX that take non-enum, non-primitive values. I'd like to eliminate them entirely to avoid user error (reusing metadata elements in multiple places). Can each of these cases be justified? Or can we add factory methods to the interfaces instead (e.g. newVersionMetadata instead of setVersionMetadata)? Then there's no need for setParent either. ComponentMetadata: setVersionMetadata setVersionMetadata setDatastoreIdentityMetadata setPrimaryKeyMetadata DiscriminatorMetadata: setIndexMetadata ElementMetadata: setEmbeddedMetadata setIndexMetadata setUniqueMetadata setForeignKeyMetadata JoinMetadata: setForeignKeyMetadata setPrimaryKeyMetadata KeyMetadata: setEmbeddedMetadata setIndexMetadata setUniqueMetadata setForeignKeyMetadata MemberMetadata: setArrayMetadata setCollectionMetadata setMapMetadata setJoinMetadata setEmbeddedMetadata setElementMetadata setKeyMetadata setValueMetadata setIndexMetadata setUniqueMetadata setForeignKeyMetadata Metadata: setParent OrderMetadata: setIndexMetadata ValueMetadata: setEmbeddedMetadata setIndexMetadata setUniqueMetadata setForeignKeyMetadata VersionMetadata: setIndexMetadata In SequenceMetadata, not sure if you intended this to be Class or String... None of the other metadata calls use Class. + * Method to set the result class name for the query + * + * @param cls Result class name + */ + void setFactoryClass(Class cls);
        Hide
        Andy Jefferson added a comment -

        > InterfaceMetadata extends both Metadata and ComponentMetadata. But

        Good catch.

        > ComponentMetadata might include get/setPersistenceModifier since presumably this applies to interfaces as well.

        Not in the DTD it doesn't. Not sure what a "persistence-aware" persistent interface would be :-S

        > There are several cases of setXXX that take [...] Then there's no need for setParent either.

        setParent was a hangover from before adding the newXXX, and can go.
        setXXX used to distinguish the single-valued child components from multiple-valued child components (newXXX); naming issue. Since some are single-valued I used setXXX since more like a bean property.

        > SequenceMetaData

        Yes, inconsistent.

        Re: priority order.

        My use-case that originated this is for classes where there is no file-based metadata. Consequently I've no problem with the wording being around that, so a user can define metadata for a class that has no existing metadata. As a result, a call to pmf.registerMetadata() with something that tries to redefine a class that already has registered metadata would result in a JDOUserException.

        Show
        Andy Jefferson added a comment - > InterfaceMetadata extends both Metadata and ComponentMetadata. But Good catch. > ComponentMetadata might include get/setPersistenceModifier since presumably this applies to interfaces as well. Not in the DTD it doesn't. Not sure what a "persistence-aware" persistent interface would be :-S > There are several cases of setXXX that take [...] Then there's no need for setParent either. setParent was a hangover from before adding the newXXX, and can go. setXXX used to distinguish the single-valued child components from multiple-valued child components (newXXX); naming issue. Since some are single-valued I used setXXX since more like a bean property. > SequenceMetaData Yes, inconsistent. Re: priority order. My use-case that originated this is for classes where there is no file-based metadata. Consequently I've no problem with the wording being around that, so a user can define metadata for a class that has no existing metadata. As a result, a call to pmf.registerMetadata() with something that tries to redefine a class that already has registered metadata would result in a JDOUserException.
        Hide
        Michael Bouschen added a comment -

        Sorry for being late in this discussion.

        The patch jdometadata5.patch looks good. However, I have a couple of remarks and most of them have been discussed during the JDO TCK conference call Friday Dec 4:

        • I like the idea that a Metadata component serves as factory method for its child components. E.g. ClassMetada provides a method to add a new field or a method to add the inheritance metadata for the class. The method has two tasks: create a new subcomponent instance and automatically attach the new instance to the parent. Given that, I think we should skip method setParent from the super interface Metadata.
        • In the current patch the naming pattern for single valued subcomponent is "Subcomponent setSubcomponent()" for the factory method and "Subcomponent getSubcomponent()" for the getter. I find this confusing, because this looks like a JavaBeans pattern where the setter method uses a very unusual signature. I propose to use "Subcomponent newSubcomponent()" for the factory methods for both single-valued and multi-valued subcomponents.
        • The factory method newSubcomponent may be called multiple times in the case of multi-valued subcomponents, each call creates a new subcomponent. In the case of a single-valued subcomponent, the first call creates the subcomponent and further call should throw an exception.
        • I propose to have two overloaded factory methods: one version taking no arguments and another version taking the required attributes of the subcomponent as parameters. E.g. ClassMetadata would have the methods:
          FieldMetadata newField();
          FieldMetadata newField(String name);
          The version w/o parameters allows a tool to create the subcomponent before processing all the attributes of the subcomponent.
        • As already included in the patch, I do not see the need for methods modifying the structure of the metadata graph after it is setup, so we do not need any unset or remove methods.
        • The methods for metadata attributes (that are not subcomponents) follow the JavaBeans pattern, e.g.
          void setColumn(String col);
          String getColumn();
        • Boolean attributes use the Java Wrapper class Boolean as the return type. This allows to return null in order to express, that the attribute has not been set. Should the corresponding setter take a Boolean argument instead of a boolean? This way getter and setter use the same type for the property.
        Show
        Michael Bouschen added a comment - Sorry for being late in this discussion. The patch jdometadata5.patch looks good. However, I have a couple of remarks and most of them have been discussed during the JDO TCK conference call Friday Dec 4: I like the idea that a Metadata component serves as factory method for its child components. E.g. ClassMetada provides a method to add a new field or a method to add the inheritance metadata for the class. The method has two tasks: create a new subcomponent instance and automatically attach the new instance to the parent. Given that, I think we should skip method setParent from the super interface Metadata. In the current patch the naming pattern for single valued subcomponent is "Subcomponent setSubcomponent()" for the factory method and "Subcomponent getSubcomponent()" for the getter. I find this confusing, because this looks like a JavaBeans pattern where the setter method uses a very unusual signature. I propose to use "Subcomponent newSubcomponent()" for the factory methods for both single-valued and multi-valued subcomponents. The factory method newSubcomponent may be called multiple times in the case of multi-valued subcomponents, each call creates a new subcomponent. In the case of a single-valued subcomponent, the first call creates the subcomponent and further call should throw an exception. I propose to have two overloaded factory methods: one version taking no arguments and another version taking the required attributes of the subcomponent as parameters. E.g. ClassMetadata would have the methods: FieldMetadata newField(); FieldMetadata newField(String name); The version w/o parameters allows a tool to create the subcomponent before processing all the attributes of the subcomponent. As already included in the patch, I do not see the need for methods modifying the structure of the metadata graph after it is setup, so we do not need any unset or remove methods. The methods for metadata attributes (that are not subcomponents) follow the JavaBeans pattern, e.g. void setColumn(String col); String getColumn(); Boolean attributes use the Java Wrapper class Boolean as the return type. This allows to return null in order to express, that the attribute has not been set. Should the corresponding setter take a Boolean argument instead of a boolean? This way getter and setter use the same type for the property.
        Hide
        Andy Jefferson added a comment -

        javax.jdo.metadata package has been commited to SVN (trunk).
        This patch is for the PMF methods - not committed since that will break compilation where the implementation doesn't yet have these methods. Uses newXXX instead of setXXX for single-valued props. Have followed java beans pattern for most, with the exception of boolean fields that have a default, so cannot be "unset". Maybe some attributes have been missed off - left as an exercise to the reader

        I don't see much point in methods like newField() - nothing in our use-cases require a FieldMetadata when we don't have the name of the field.

        Show
        Andy Jefferson added a comment - javax.jdo.metadata package has been commited to SVN (trunk). This patch is for the PMF methods - not committed since that will break compilation where the implementation doesn't yet have these methods. Uses newXXX instead of setXXX for single-valued props. Have followed java beans pattern for most, with the exception of boolean fields that have a default, so cannot be "unset". Maybe some attributes have been missed off - left as an exercise to the reader I don't see much point in methods like newField() - nothing in our use-cases require a FieldMetadata when we don't have the name of the field.
        Hide
        Michael Bouschen added a comment -

        I agree to add the PMF changes later.

        About the boolean properties:
        I think the rule is, the getter method returns a Boolean for a property that does not have a default. The method returns null if the property has not been set. The getter method returns a boolean (primitive type) if the property has a default, then the property always has a value. I found two cases where I propose to change the return type (as part of the exercise :

        • FetchGroupMetadata: Boolean getPostLoad();
        • IndexMetadata: boolean getUnique();
          The attached patch BooleanProperties.patch changes the above signatures.

        About the newXXX method w/o arguments:
        I agree to not add these methods, because it would bloat the interfaces. However, I think it should be legal to pass null when constructing the instances as along as the required values are defined when the metadata is registered (using method registerMetadata).

        Show
        Michael Bouschen added a comment - I agree to add the PMF changes later. About the boolean properties: I think the rule is, the getter method returns a Boolean for a property that does not have a default. The method returns null if the property has not been set. The getter method returns a boolean (primitive type) if the property has a default, then the property always has a value. I found two cases where I propose to change the return type (as part of the exercise : FetchGroupMetadata: Boolean getPostLoad(); IndexMetadata: boolean getUnique(); The attached patch BooleanProperties.patch changes the above signatures. About the newXXX method w/o arguments: I agree to not add these methods, because it would bloat the interfaces. However, I think it should be legal to pass null when constructing the instances as along as the required values are defined when the metadata is registered (using method registerMetadata).
        Hide
        Craig L Russell added a comment -

        > - FetchGroupMetadata: Boolean getPostLoad();
        > - IndexMetadata: boolean getUnique();
        > The attached patch BooleanProperties.patch changes the above signatures.

        I agree with this patch. These are now consistent with the B vs. b rule and it's ok by me to check in.

        > About the newXXX method w/o arguments:
        > I agree to not add these methods, because it would bloat the interfaces. However, I think it should be legal to pass null when constructing the instances as along as the required values are defined when the metadata is registered (using method registerMetadata).

        This is a good compromise. If building the metadata in a program, or responding to UI input, the required fields are known up front. But for deferred required processing, the use-case is tools that parse an xml file and build the model while parsing. For example, when the start-element for field is reached, the name attribute hasn't been processed, so the parser handler needs to create a field metadata without knowing the name.

        But by the time the metadata is registered for use, the required features need to be valid. I'll note this in the spec.

        Show
        Craig L Russell added a comment - > - FetchGroupMetadata: Boolean getPostLoad(); > - IndexMetadata: boolean getUnique(); > The attached patch BooleanProperties.patch changes the above signatures. I agree with this patch. These are now consistent with the B vs. b rule and it's ok by me to check in. > About the newXXX method w/o arguments: > I agree to not add these methods, because it would bloat the interfaces. However, I think it should be legal to pass null when constructing the instances as along as the required values are defined when the metadata is registered (using method registerMetadata). This is a good compromise. If building the metadata in a program, or responding to UI input, the required fields are known up front. But for deferred required processing, the use-case is tools that parse an xml file and build the model while parsing. For example, when the start-element for field is reached, the name attribute hasn't been processed, so the parser handler needs to create a field metadata without knowing the name. But by the time the metadata is registered for use, the required features need to be valid. I'll note this in the spec.
        Hide
        Andy Jefferson added a comment -

        All setters should be changed to return the object itself, hence allowing
        ColumnMetadata colmd = fmd.newColumnMetadata();
        colmd.setLength(24).setScale(6).setJdbcType("FLOAT").setName("MY_COL");

        Show
        Andy Jefferson added a comment - All setters should be changed to return the object itself, hence allowing ColumnMetadata colmd = fmd.newColumnMetadata(); colmd.setLength(24).setScale(6).setJdbcType("FLOAT").setName("MY_COL");
        Hide
        Craig L Russell added a comment -

        > All setters should be changed to return the object itself, hence allowing

        Good idea.

        Show
        Craig L Russell added a comment - > All setters should be changed to return the object itself, hence allowing Good idea.
        Hide
        Michael Bouschen added a comment -

        Thanks for checking in the patch.

        Show
        Michael Bouschen added a comment - Thanks for checking in the patch.
        Hide
        Andy Jefferson added a comment -

        I've now made some minor corrections to the API and checked them in. I've also checked in the additions to the PMF. There is also an implementation of
        PMF.newMetadata
        PMF.registerMetadata
        in DataNucleus SVN trunk.

        Show
        Andy Jefferson added a comment - I've now made some minor corrections to the API and checked them in. I've also checked in the additions to the PMF. There is also an implementation of PMF.newMetadata PMF.registerMetadata in DataNucleus SVN trunk.
        Hide
        Andy Jefferson added a comment -

        Proposal to make the interface more typesafe, and more usable. Methods to add to supplement the existing methods.

        PackageMetadata JDOMetadata.newPackageMetadata(Class cls); // takes the package from the class
        ClassMetadata JDOMetadata.newClassMetadata(Class cls); // Add package if needed then the class
        InterfaceMetadata JDOMetadata.newInterfaceMetadata(Class cls); // Add package if needed then the class
        PackageMetadata.newClassMetadata(Class cls); // takes the class name from the class
        PackageMetadata.newInterfaceMetadata(Class cls); // takes the interface name from the class

        ClassMetadata.newFieldMetadata(Field fld); // takes the field name from the field
        ClassMetadata.newPropertyMetadata(Method meth); // takes the property name from the method (bean-style)

        so then users code is refactorable. Obviously for the latter two methods the user still has to obtain the Field/Method so of limited benefit for those.

        Show
        Andy Jefferson added a comment - Proposal to make the interface more typesafe, and more usable. Methods to add to supplement the existing methods. PackageMetadata JDOMetadata.newPackageMetadata(Class cls); // takes the package from the class ClassMetadata JDOMetadata.newClassMetadata(Class cls); // Add package if needed then the class InterfaceMetadata JDOMetadata.newInterfaceMetadata(Class cls); // Add package if needed then the class PackageMetadata.newClassMetadata(Class cls); // takes the class name from the class PackageMetadata.newInterfaceMetadata(Class cls); // takes the interface name from the class ClassMetadata.newFieldMetadata(Field fld); // takes the field name from the field ClassMetadata.newPropertyMetadata(Method meth); // takes the property name from the method (bean-style) so then users code is refactorable. Obviously for the latter two methods the user still has to obtain the Field/Method so of limited benefit for those.
        Hide
        Richard Schilling added a comment -

        This is a great approach. Looks like all the elements are covered.

        Show
        Richard Schilling added a comment - This is a great approach. Looks like all the elements are covered.
        Hide
        Michael Bouschen added a comment -

        I like the idea taking Class instances as an argument.

        However, I propose that the method creating the package metadata takes a java.lang.Package argument (instead of a Class instance):
        PackageMetadata JDOMetadata.newPackageMetadata(java.lang.Package pkg);
        It's easy to get the Package instance from a Class instance: cls.getPackage().

        Show
        Michael Bouschen added a comment - I like the idea taking Class instances as an argument. However, I propose that the method creating the package metadata takes a java.lang.Package argument (instead of a Class instance): PackageMetadata JDOMetadata.newPackageMetadata(java.lang.Package pkg); It's easy to get the Package instance from a Class instance: cls.getPackage().
        Hide
        Andy Jefferson added a comment -

        Michael's suggestion taking in Package was included in SVN a few days ago, as were the other methods. Also added the methods newMetadata()/registerMetadata() to JDOEnhancer so it can be plugged into the enhancement process.

        Show
        Andy Jefferson added a comment - Michael's suggestion taking in Package was included in SVN a few days ago, as were the other methods. Also added the methods newMetadata()/registerMetadata() to JDOEnhancer so it can be plugged into the enhancement process.
        Hide
        Andy Jefferson added a comment -

        Anything more needed on this JIRA ? Is it in the spec ?

        Show
        Andy Jefferson added a comment - Anything more needed on this JIRA ? Is it in the spec ?
        Hide
        Matthew T. Adams added a comment -

        Naming comment. I suggest renaming ComponentMetadata to TypeMetadata. "Component" is much more overloaded term than "type", IMHO: anything can be a component (class, widget, service, system, subsystem, etc), but classes and interfaces are types. See patch.

        Sorry for not getting to this so late in the game.

        Show
        Matthew T. Adams added a comment - Naming comment. I suggest renaming ComponentMetadata to TypeMetadata. "Component" is much more overloaded term than "type", IMHO: anything can be a component (class, widget, service, system, subsystem, etc), but classes and interfaces are types. See patch. Sorry for not getting to this so late in the game.
        Hide
        Craig L Russell added a comment -

        > I suggest renaming ComponentMetadata to TypeMetadata.

        I agree with this suggestion. Component is way too overloaded, and Type is precisely what we are defining with this API.

        Show
        Craig L Russell added a comment - > I suggest renaming ComponentMetadata to TypeMetadata. I agree with this suggestion. Component is way too overloaded, and Type is precisely what we are defining with this API.
        Hide
        Michael Bouschen added a comment -

        I agree with the renaming, TypeMetadata is the better name.

        I tried your patch, but I could not apply the patch. Here is the error message:
        can't find file to patch at input line 18
        Perhaps you used the wrong -p or --strip option?
        The text leading up to this was:
        --------------------------

        Index: TypeMetadata.java
        ===================================================================
        — TypeMetadata.java (revision 776348)
        +++ TypeMetadata.java (working copy)

        It looks like the patch misses the new interface TypeMetadata.

        Can you please double check and create a new patch? Thanks.

        Show
        Michael Bouschen added a comment - I agree with the renaming, TypeMetadata is the better name. I tried your patch, but I could not apply the patch. Here is the error message: can't find file to patch at input line 18 Perhaps you used the wrong -p or --strip option? The text leading up to this was: -------------------------- Index: TypeMetadata.java =================================================================== — TypeMetadata.java (revision 776348) +++ TypeMetadata.java (working copy) It looks like the patch misses the new interface TypeMetadata. Can you please double check and create a new patch? Thanks.
        Hide
        Matthew T. Adams added a comment -

        Will retry patch. Stand by for new one.

        Show
        Matthew T. Adams added a comment - Will retry patch. Stand by for new one.
        Hide
        Matthew T. Adams added a comment -

        Sorry for the delay – I had some svn 1.6 v. 1.5 problems. Please comment on issue if new patch (type-2.patch) has issues.

        Show
        Matthew T. Adams added a comment - Sorry for the delay – I had some svn 1.6 v. 1.5 problems. Please comment on issue if new patch (type-2.patch) has issues.
        Hide
        Michelle Caisse added a comment -

        Patch applies fine for me but I get:

        java:compile:
        [echo] Compiling to c:\jdonew\trunk\api2/target/classes
        [javac] Compiling 4 source files to C:\jdonew\trunk\api2\target\classes
        C:\jdonew\trunk\api2\src\java\javax\jdo\metadata\ComponentMetadata.java:27: class TypeMetadata is public, should be declared in a file named TypeMetadata.java
        public interface TypeMetadata extends Metadata {
        ^
        1 error

        Show
        Michelle Caisse added a comment - Patch applies fine for me but I get: java:compile: [echo] Compiling to c:\jdonew\trunk\api2/target/classes [javac] Compiling 4 source files to C:\jdonew\trunk\api2\target\classes C:\jdonew\trunk\api2\src\java\javax\jdo\metadata\ComponentMetadata.java:27: class TypeMetadata is public, should be declared in a file named TypeMetadata.java public interface TypeMetadata extends Metadata { ^ 1 error
        Hide
        Matthew T. Adams added a comment -

        So it looks like the patch program has difficulty with patch files created via 'svn diff ...' that include a renamed file. I'd prefer that DataNucleus make the change when they're ready and then commit rather than bother with svn diff with renamed files (plus my 1.6/1.5 working copy version issues).

        Show
        Matthew T. Adams added a comment - So it looks like the patch program has difficulty with patch files created via 'svn diff ...' that include a renamed file. I'd prefer that DataNucleus make the change when they're ready and then commit rather than bother with svn diff with renamed files (plus my 1.6/1.5 working copy version issues).
        Hide
        Michael Bouschen added a comment -

        I see the same problem as described by Michelle.

        However, I think the patch file is ok to start with. After applying the patch you simply need to rename ComponentMetadata.java to TypeMetadata.java:
        svn mv api2/src/java/javax/jdo/metadata/ComponentMetadata.java api2/src/java/javax/jdo/metadata/TypeMetadata
        With this change I can build api2 w/o problems.

        I tried creating a patch file after I "svn renamed" the file. But applying the resulting patch file runs into the same problem I faced when trying Matthew's first patch type-patch. It fails to update TypeMetadata.java.

        So I think the best way is to use type-2.patch, svn rename ComponentMetadata and then check in the result.

        Show
        Michael Bouschen added a comment - I see the same problem as described by Michelle. However, I think the patch file is ok to start with. After applying the patch you simply need to rename ComponentMetadata.java to TypeMetadata.java: svn mv api2/src/java/javax/jdo/metadata/ComponentMetadata.java api2/src/java/javax/jdo/metadata/TypeMetadata With this change I can build api2 w/o problems. I tried creating a patch file after I "svn renamed" the file. But applying the resulting patch file runs into the same problem I faced when trying Matthew's first patch type-patch. It fails to update TypeMetadata.java. So I think the best way is to use type-2.patch, svn rename ComponentMetadata and then check in the result.
        Hide
        Matthew T. Adams added a comment -

        Some names contained upper case Ds in the use of term metadata. This patch corrects that.

        Show
        Matthew T. Adams added a comment - Some names contained upper case Ds in the use of term metadata. This patch corrects that.
        Hide
        Craig L Russell added a comment -

        > Some names contained upper case Ds in the use of term metadata. This patch corrects that.

        Good catch. Names need to be consistent.

        Show
        Craig L Russell added a comment - > Some names contained upper case Ds in the use of term metadata. This patch corrects that. Good catch. Names need to be consistent.
        Hide
        Andy Jefferson added a comment -

        Patches applied

        Show
        Andy Jefferson added a comment - Patches applied
        Hide
        Craig L Russell added a comment -

        Thanks for applying the patches.

        Are we ready to close this one?

        +1 from me.

        Show
        Craig L Russell added a comment - Thanks for applying the patches. Are we ready to close this one? +1 from me.
        Hide
        Andy Jefferson added a comment -

        It's in api2, It's in the spec. Any plans for TCK tests ?

        Show
        Andy Jefferson added a comment - It's in api2, It's in the spec. Any plans for TCK tests ?
        Hide
        Craig L Russell added a comment -

        Of course we need TCK tests. I don't know what I was thinking.

        Show
        Craig L Russell added a comment - Of course we need TCK tests. I don't know what I was thinking.
        Hide
        Renato Garcia added a comment -

        I think IndexMetadata.getUnique has been changed by mistake here http://svn.apache.org/viewvc/db/jdo/branches/2.3-ea/api2/src/java/javax/jdo/metadata/IndexMetadata.java?view=diff&r1=737665&r2=724172&diff_format=u

        /**

        • Accessor for whether unique.
        • @return Unique?
          */
        • Boolean getUnique();
          + boolean getUnique();
        Show
        Renato Garcia added a comment - I think IndexMetadata.getUnique has been changed by mistake here http://svn.apache.org/viewvc/db/jdo/branches/2.3-ea/api2/src/java/javax/jdo/metadata/IndexMetadata.java?view=diff&r1=737665&r2=724172&diff_format=u /** Accessor for whether unique. @return Unique? */ Boolean getUnique(); + boolean getUnique();
        Hide
        Andy Jefferson added a comment -

        You're referring to a change some 7 months ago, and you think it is incorrect ?
        No it wasn't changed "by mistake". The DTD (for "index.unique") says
        <!ATTLIST index unique (true|false) 'false'>
        hence that is a boolean and has a default of false. Hence there is no way it can be Boolean. QED

        Show
        Andy Jefferson added a comment - You're referring to a change some 7 months ago, and you think it is incorrect ? No it wasn't changed "by mistake". The DTD (for "index.unique") says <!ATTLIST index unique (true|false) 'false'> hence that is a boolean and has a default of false. Hence there is no way it can be Boolean. QED
        Hide
        Renato Garcia added a comment -

        Sorry, I should have said "MIGHT have been changed by mistake", and I couldn't realize how to determine the defaults, in order to see if this was really wrong.

        Well, this is an early access version, and by reading this thread it sounds that the tests aren't ready yet. Besides that, I have a NPE from a autobox conversion using DataNucleus(as you know, is the only implementation that is 2.3 compatible) which also doesn't have tests for this. So, 7 months isn't making a lot of difference here I believe.

        I'll post my finds at DN forums so you can take a look.

        Thanks for your clarification anyway!

        Show
        Renato Garcia added a comment - Sorry, I should have said "MIGHT have been changed by mistake", and I couldn't realize how to determine the defaults, in order to see if this was really wrong. Well, this is an early access version, and by reading this thread it sounds that the tests aren't ready yet. Besides that, I have a NPE from a autobox conversion using DataNucleus(as you know, is the only implementation that is 2.3 compatible) which also doesn't have tests for this. So, 7 months isn't making a lot of difference here I believe. I'll post my finds at DN forums so you can take a look. Thanks for your clarification anyway!
        Hide
        Andy Jefferson added a comment -

        DataNucleus actually does have the start of tests for JDO2.3 Metadata and, as since 2003, has been open for contributions. If you have tests to contribute for this feature then why not raise a JIRA (on DN JIRA) and contribute the tests there (using the existing DN sample classes), with patches for
        http://datanucleus.svn.sourceforge.net/viewvc/datanucleus/test/accessplatform/trunk/test.jdo.general/

        Show
        Andy Jefferson added a comment - DataNucleus actually does have the start of tests for JDO2.3 Metadata and, as since 2003, has been open for contributions. If you have tests to contribute for this feature then why not raise a JIRA (on DN JIRA) and contribute the tests there (using the existing DN sample classes), with patches for http://datanucleus.svn.sourceforge.net/viewvc/datanucleus/test/accessplatform/trunk/test.jdo.general/
        Hide
        Renato Garcia added a comment -

        Done. Patch attached with test and a possible fix -> http://www.jpox.org/servlet/jira/browse/NUCCORE-338

        Show
        Renato Garcia added a comment - Done. Patch attached with test and a possible fix -> http://www.jpox.org/servlet/jira/browse/NUCCORE-338
        Hide
        Craig L Russell added a comment -

        Checked in test case.

        svn commit -m "JDO-615 Add getMetadata test" src/conf/pmf.conf src/java/org/apache/jdo/tck/api/persistencemanagerfactory/metadata
        Sending src/conf/pmf.conf
        Adding src/java/org/apache/jdo/tck/api/persistencemanagerfactory/metadata
        Adding src/java/org/apache/jdo/tck/api/persistencemanagerfactory/metadata/GetMetadataTest.java

        Show
        Craig L Russell added a comment - Checked in test case. svn commit -m " JDO-615 Add getMetadata test" src/conf/pmf.conf src/java/org/apache/jdo/tck/api/persistencemanagerfactory/metadata Sending src/conf/pmf.conf Adding src/java/org/apache/jdo/tck/api/persistencemanagerfactory/metadata Adding src/java/org/apache/jdo/tck/api/persistencemanagerfactory/metadata/GetMetadataTest.java
        Hide
        Andy Jefferson added a comment -

        Complete ?

        Show
        Andy Jefferson added a comment - Complete ?
        Hide
        Craig L Russell added a comment -

        Resolved.

        Show
        Craig L Russell added a comment - Resolved.

          People

          • Assignee:
            Craig L Russell
            Reporter:
            Andy Jefferson
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development