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