Hi Tom,
Thanks for getting back so soon. Pl. see my reply in line...
Tom Ware wrote:
> Hi Sahoo,
>
> I am working away at learning how this code works. Here are a few
> initial comments. I will send more when I have learned it all.
>
> - our coding convention requires braces around all blocks even if they
> are a single line. Where are some examples where there are issues:
> oracle\toplink\essentials\ejb\cmp3\persistence\DirectoryArchive.java - 92
> oracle\toplink\essentials\ejb\cmp3\persistence\JarInputStreamURLArchive.java
> - 93
>
Fixed those files. Let me know if you find more.
> - There are a number of exceptions we do not expect to get to that are
> not internationalized. Can we either internationalize them or use the
> strategy used in DirectoryInsideJarArchive an use assertions instead
> of exceptions.
Changed them to use assert.
>
> -
> oracle\toplink\essentials\ejb\cmp3\persistence\DirectoryInsideJarURLArchive.java
> - The comment indicates this file is covered by a different license.
> If this is true, it could be an issue. Can this file be covered by CDDL?
>
Now changed. That was an oversight at my end. Thanks for catching.
> - Several classes have import statements reordered and several classes
> have explicit import statements (x.y.z) changed to import x.y.*. Is
> this the result of some kind of tool use? If we are going to choose to
> do something like this, we should do it for the whole of essentials
> rather than file by file.
Yes it was done by a tool. I have not been able to find any pattern in
the existing import order, so I changed them. Now I have reversed such
changes in the files where it was done. The files affected are:
MetadataProject.java, MetadataProcessor.java, SEPersistenceUnitInfo,
PersistenceUnitProcessor, EntityManagareSetupImpl.
Is the import order well defined in the code base?
>
> -
> oracle\toplink\essentials\internal\ejb\cmp3\metadata\MetadataProcessor.java
> - 80 - // TODO: Sahoo: fix this code path - what is this?
I didn't see any use of that constructor in the entire GF code base and
the comment says some test suite calls that. You or someone from your
end has to tell me how/when that constructor gets called? Then I can fix
that code path. Let me know, if we can remove that constructor. I don't
understand how it is supposed to work without a persistence unit info.
>
> I'll send more comments soon,
I will send you the changed files after that.
Thanks,
Sahoo
> -Tom
>
>
> Sanjeeb Kumar Sahoo wrote:
>
>> (resending with jar file attachment instead of zip file)
>>
>> Hi Tom, Guy,
>>
>> Attached here with are the fixes for following issues:
>> https://glassfish.dev.java.net/issues/show_bug.cgi?id=1051
>> https://glassfish.dev.java.net/issues/show_bug.cgi?id=1074
>> https://glassfish.dev.java.net/issues/show_bug.cgi?id=1115
>> https://glassfish.dev.java.net/issues/show_bug.cgi?id=1131
>> https://glassfish.dev.java.net/issues/show_bug.cgi?id=1206
>>
>> They are all related issues, so I am fixing them together. In the
>> process I am also refactoring the PU processing code a bit. List of
>> files affected are: (A stands for new files and M stands for modified
>> files)
>> A src/java/oracle/toplink/essentials/ejb/cmp3/persistence/Archive.java
>> A
>> src/java/oracle/toplink/essentials/ejb/cmp3/persistence/ArchiveFactoryImpl.java
>>
>> A
>> src/java/oracle/toplink/essentials/ejb/cmp3/persistence/DirectoryArchive.java
>>
>> A
>> src/java/oracle/toplink/essentials/ejb/cmp3/persistence/DirectoryInsideJarURLArchive.java
>>
>> A
>> src/java/oracle/toplink/essentials/ejb/cmp3/persistence/JarFileArchive.java
>>
>> A
>> src/java/oracle/toplink/essentials/ejb/cmp3/persistence/JarInputStreamURLArchive.java
>>
>> M src/java/oracle/toplink/essentials/ejb/cmp3/StaticWeaver.java
>> M
>> src/java/oracle/toplink/essentials/ejb/cmp3/persistence/PersistenceUnitProcessor.java
>>
>> M src/java/oracle/toplink/essentials/exceptions/ValidationException.java
>> M
>> src/java/oracle/toplink/essentials/exceptions/i18n/ValidationExceptionResource.java
>>
>> M
>> src/java/oracle/toplink/essentials/internal/ejb/cmp3/EntityManagerSetupImpl.java
>>
>> M
>> src/java/oracle/toplink/essentials/internal/ejb/cmp3/JavaSECMPInitializer.java
>>
>>
>> M
>> src/java/oracle/toplink/essentials/internal/ejb/cmp3/metadata/MetadataProcessor.java
>>
>> M
>> src/java/oracle/toplink/essentials/internal/ejb/cmp3/metadata/MetadataProject.java
>>
>> M
>> src/java/oracle/toplink/essentials/internal/ejb/cmp3/metadata/MetadataValidator.java
>>
>> M
>> src/java/oracle/toplink/essentials/internal/ejb/cmp3/xml/XMLHelper.java
>>
>> What are the new files:
>> The new files (Archive, ArchiveFactoryImpl and various ArchiveImpls) are
>> to do with URL handling. They offer functionality like listing entries
>> in a URL, getting a resource from a URL etc. This abstraction helps us
>> in centralizing URL handling code to a few known classes. They have been
>> extensively tested in various environment. Archive and
>> ArchiveFactoryImpl are the starting point classes. Please refer to the
>> javadocs in all those classes for more information. They can be tested
>> in standalone Java application.
>>
>> How are they used?
>> Most of the code in PersistenceUnitProcessor and MetadataProcessor now
>> uses Archive interface as opposed to URL interface.
>>
>> Changes to existing files:
>> src/java/oracle/toplink/essentials/internal/ejb/cmp3/JavaSECMPInitializer.java:
>>
>> Uses Archive interface as opposed to URL.
>> src/java/oracle/toplink/essentials/internal/ejb/cmp3/EntityManagerSetupImpl.java:
>>
>> The metadata processing code from this class have been moved to
>> MetadataProcessor. It plays the role of a director in builder pattern.
>> It uses MetadataProcessor to build MetadataProject object.
>> src/java/oracle/toplink/essentials/internal/ejb/cmp3/metadata/MetadataProcessor.java:
>>
>> A lot of code have been moved from EntityManagerSetupImpl to this class.
>> Also, it now follows builder pattern. It acts as the Builder while
>> EntityManagerSetupImpl is the director and MetadataProject is the
>> product.
>> src/java/oracle/toplink/essentials/internal/ejb/cmp3/metadata/MetadataProject.java:
>>
>> To avoid repeated reading of mapping files, annotations etc, we now
>> store Document object for each mapping file in MetadataProject. We also
>> store entity class names here. That allows us to use the same set of
>> class names in predeploy and deploy step.
>> src/java/oracle/toplink/essentials/ejb/cmp3/persistence/PersistenceUnitProcessor.java:
>>
>> URL processing code have been moved to various Archive implementation
>> classes.
>> src/java/oracle/toplink/essentials/ejb/cmp3/StaticWeaver.java: Trivial
>> changed required to accommodate API change in PersistenceUnitProcessor.
>>
>> Changes to Validator and Exception classes have to do with decision to
>> throw exception when a mapping file is not found or multiple mapping
>> files are found(issue 1051 & 1074).
>>
>> The following files are changed to address
>> https://glassfish.dev.java.net/issues/show_bug.cgi?id=1206:
>> src/java/oracle/toplink/essentials/internal/ejb/cmp3/xml/parser/PersistenceContentHandler.java:
>>
>> When jar-file element value is read from xml stream, it is stored in an
>> instance variable in SEPersistenceUnitInfo.
>> src/java/oracle/toplink/essentials/ejb/cmp3/persistence/SEPersistenceUnitInfo.java:
>>
>> URL corresponding to each jar-file is created and returned in
>> getJarFileUrls() method.
>>
>>
>> Testing done so far:
>> --------------------------
>> 1) entity-persistence-tests pass after setting
>> toplink.orm.throw.exceptions=false in config/META-INF/persistence.xml.
>> 2) Unit tests for all the bugs fixed here.
>> 3) successful run of TCK
>> 4) GF QuickLook tests
>> 5) JPA packaging tests in verifier that extensively test PU processing
>> code in TopLink.
>> 6) Testing where file name contains space.
>> 7) Testing JPA application in Tomcat
>> 8) Running JPA apps in GlassFish
>>
>> In one of the bug notes you mentioned about testing inside Oracle AS. I
>> don't have access to any such set up. If someone at your end is ready to
>> run some tests in Oracle AS, then I am happy to provide a binary patch
>> to her or him.
>>
>> Thanks,
>> Sahoo
>>
>>
>>
>