I just realized that I had not checked in the changes to
entity-persistence-tests/config/META-INF/persistence.xml where we have
to set toplink.orm.throw.exceptions = false. I made this change and
checked in as well.
I am in the process of adding some unit test cases for the bugs that I
have fixed. I will send them for review.
Thanks,
Sahoo
Sanjeeb Kumar Sahoo wrote:
> 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
>>>>>
>>>>>
>>>>>
>>>>>
>>