Hi Sahoo,
'Just a couple more comments and you should be able to check in.
First, your questions:
1. There is not currently an enforced order to the imports. I am not
opposed to having one, but I think we should all agree on it and make
the change to the whole code base rather than change it on a file by
file basis.
2. The
oracle\toplink\essentials\internal\ejb\cmp3\metadata\MetadataProcessor.java
80 - // TODO: Sahoo: fix this code path: Could you add @deprected to
this method? There is an Oracle-internal test that uses this method.
We will have to update it.
Next, a couple more comments:
1. (not a problem with your code, but since we're already changing the
method:) Maybe we should change JavaSECMPInitializer.initialize() to
store the persistence archives before the for loop rather than calculate
them each time. - If you want to leave this for another time, that is fine.
2. MetadataProcessor. There are still a number of System.out.printlns
3. MetadataProcessor. There are still a number of throw RuntimeException().
-Tom
Sanjeeb Kumar Sahoo wrote:
>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
>>>
>>>
>>>
>>>
>>>
--
Tom Ware
Principal Software Engineer
Oracle Canada Inc.
Direct: (613) 783-4598
Email: tom.ware_at_oracle.com