Hi Tom,
1. Changed JavaSECmpInitializer as per your suggestion.
2. Replaced System.out by AbstractSession.logMessage() in MetadataProcessor
3. There are only two RuntimeExceptions in MetadataProcessor and they
will continue to be so as they are not expected statically. I have now
made necessary changes so that they are not directly thrown from code.
Instead they are passed to
AbstractSession.handleException(RuntimeException e).
4. Deprecated that constructor in MetadataConstructor. It is also fixed
now and can be used from the test suite.
5. I made one more small change which I should have done earlier. I have
now added a new field called /AbstractSession m_session;/ in
MetadataProcessor. It is quite logical because a processor is associated
with only session and this field is initialized in the constructors
which were already receiving an AbstractSession as their argument. After
this change, the methods of MetadataProcessor no more accepts
AbstractSession as an argument. They always operate in the context of
m_session.
With all these changes, entity-persistence-tests have passed and so have
the unit tests for the bugs. To avoid any potential merging issues, I
have checked in the changes.
Thanks a lot for sending your review comments in a quick time.
Thanks,
Sahoo
Tom Ware wrote:
> 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
>>>>
>>>>
>>>>
>>>>
>