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
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>
>
- application/octet-stream attachment: 238.jar