Hi Marina,
The code changes look good. Assuming your test cases pass and the
entity-persistence tests pass, you should be good to check-in.
-Tom
Marina Vatkina wrote:
>Hi Tom,
>
>Please review the changes. They include the previous changes to issue
>238 as well (holidays and broken tests kept me from checking it in).
>
>I did the cleanup and tested both cases - a Java SE test located in
>a directory with spaces and a Java EE with a directory deployment.
>entity-persistence-tests all
>
>Tom Ware wrote On 02/22/06 12:42,:
>
>
>>Hi Marina,
>>
>> I took a look at the code you sent earlier and assuming it fixes the
>>issue, it looks good.
>>
>> Additional comments inline.
>>
>>-Tom
>>
>>Marina Vatkina wrote:
>>
>>
>>
>>
>>>Hi Tom,
>>>
>>>This is really interesting, and explains why Petr reported that it
>>>worked before.
>>>While most of the cases used correctly file.toURI().toURL(),
>>>this particular place (processDirectory()) called file.toURL().
>>>The changes in v1.6 replaced all file/url access, fixing one case
>>>but causing regression in the others.
>>>
>>>Now I have follow up questions:
>>>
>>>1) null/empty-string path comparison.
>>>Do you think we should still keep them? This assertion was added
>>>as part of the changes 1.5->1.6:
>>>http://fisheye5.cenqua.com/viewrep/glassfish/entity-persistence/src/java/oracle/toplink/essentials/ejb/cmp3/persistence/PersistenceUnitProcessor.java?r1=1.5&r2=1.6
>>>
>>>
>>>
>>>
>>The answer to this question really depends on what the final
>>implementation looks like. As long as there is no danger of null or
>>empty paths in the code in the final implementation, they can be removed.
>>
>>
>>
>>
>>>2) there are several unnecessary assertions and conversions
>>>when one method calls another in the same class, e.g:
>>>
>>>processPersistenceArchive -> processDirectory
>>>
>>>getClassNamesFromURL-> getClassNamesFromJar
>>>getPersistentClassNamesFromJar-> getClassNamesFromJar
>>>
>>>Was this done on purpose? Or should I clean this up as part
>>>of my changes?
>>>
>>>
>>>
>>>
>>>
>>I am not exactly sure which assertions and conversion you are talking
>>about, but if you find they are actually unnecessary, feel free to take
>>them out.
>>
>>
>>
>>
>>>3) what is the reason for keeping such internal methods public
>>>or protected?
>>>
>>>
>>>
>>>
>>When this class was written, there was the possibility there would a
>>subclass. The public/protected methods were written at that time. The
>>work to make changes back to lower levels of access has not reached the
>>top of the priority list. Fell free to make those changes. At this
>>point, any calls into PersistenceUnitProcessor should be pretty much
>>finalized, so a compile will tell you if there are any issues with your
>>changes.
>>
>>
>>
>>
>>>thanks,
>>>-marina
>>>
>>>Tom Ware wrote On 02/22/06 06:19,:
>>>
>>>
>>>
>>>
>>>
>>>>Hi Marina,
>>>>
>>>> When we fixed a similar issue (184) a while ago, we were having
>>>>problems with the URI class handling directories with spaces. If you
>>>>take a look at the difference between version 1.5 and version 1.6 in
>>>>CVS, you will be able to see the changes we made.
>>>>
>>>>If you can make the URI version work for both issues, I think the
>>>>suggested fix should be fine.
>>>>
>>>>If there are appropriate PersistenceUnitLoadingExceptions, feel free
>>>>to reuse them. Otherwise, adding a new error code is a good idea.
>>>>
>>>>-Tom
>>>>
>>>>Marina Vatkina wrote:
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>>Hi Tom,
>>>>>
>>>>>To fix issue 228 in all of the PersistenceUnitProcessor.java, I need
>>>>>a) to change in PersistenceUnitProcessor.java the code from:
>>>>>
>>>>> String filePath = url.getFile();
>>>>> ...
>>>>> File file = new File(filePath);
>>>>>
>>>>>to
>>>>>
>>>>> File file = new File(url.toURI());
>>>>>
>>>>>b) Wrap URISyntaxException that can be thrown from URL.toURI().
>>>>>
>>>>>See the latest comments to the issue
>>>>>https://glassfish.dev.java.net/issues/show_bug.cgi?id=228
>>>>>and the attached file as the 1st cut, where duplicate code is moved to a
>>>>>separate private static method.
>>>>>
>>>>>Should I reuse any of the existing PersistenceUnitLoadingException exceptions or
>>>>>add a new one?
>>>>>
>>>>>Do you see any problems with the proposed changes?
>>>>>
>>>>>thanks,
>>>>>-marina
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>>
>>
>>
--
Tom Ware
Principal Software Engineer
Oracle Canada Inc.
Direct: (613) 783-4598
Email: tom.ware_at_oracle.com