Skip to content
This repository was archived by the owner on May 28, 2018. It is now read-only.

Conversation

@Stephan202
Copy link
Contributor

This PR aims to correct the fix for JERSEY-2621 (cc89c97). It also addresses some other edge cases. Details are in the commit message:

Overview:

  • Previously JarFileScanner assumed backslashes to be the platform independent file separator for zip files. This is incorrect (and in fact, the associated unit tests did assume forward slashes, using "javax/ws/rs" to denote the package "javax.ws.rs"). See [1] for details.

  • As a result the non-recursive scanning mode was broken: failing to find the backslash delimiter, the scanner would return classes from subpackages even when it was instructed not to do so. This has been fixed.

  • The scanner also did not verify that "matching" classes were in a proper subdirectory of the indicated package. As a result a class such as javax.ws.rs.GET was matched for the following "packages":

    • javax/ws/r (too short, misses an "s")
    • javax/ws/rs/GE (too long, includes class name prefix)

    This has been fixed as well.

The class' unit tests were extended to cover the problematic cases. Of the five new unit tests, three fail prior to the fix.

[1] https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT

NB: I already signed the OCA, in the context of #80.

Overview:
- Previously JarFileScanner assumed backslashes to be the platform independent
  file separator for zip files. This is incorrect (and in fact, the associated
  unit tests did assume forward slashes, using "javax/ws/rs" to denote the
  package "javax.ws.rs"). See [1] for details.
- As a result the non-recursive scanning mode was broken: failing to find the
  backslash delimiter, the scanner would return classes from subpackages even
  when it was instructed not to do so. This has been fixed.
- The scanner also did not verify that "matching" classes were in a proper
  subdirectory of the indicated package. As a result a class such as
  javax.ws.rs.GET was matched for the following "packages":
    - javax/ws/r (too short, misses an "s")
    - javax/ws/rs/GE (too long, includes class name prefix)
  This has been fixed as well.

The class' unit tests were extended to cover the problematic cases. Of the
five new unit tests, three fail prior to the fix.

[1] https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT
@jerseyrobot
Copy link
Contributor

Can one of the admins verify this patch?

@AdamLindenthal
Copy link
Member

Jenkins, please test this patch.

@AdamLindenthal
Copy link
Member

Hi Stephan, thanks for contributing.
Nice, that you already have the OCA signed. I assume you already know the process as well. I just triggered the build just to see, that the change does not break anything anywhere and then we will look at the code and review and eventually merge it.

Regards,
Adam

@Stephan202
Copy link
Contributor Author

Hi @AdamLindenthal , thanks for having a look. The build passed, so that's a good first sign. Just let me know in case you'd like me to do things differently.

@AdamLindenthal
Copy link
Member

EDIT: Please ignore (erroneous comment, left here just for continuity with the comment bellow)

Hi Stephan,
unfortunately, my original thought was correct. We need also you to sign the Oracle Contributor Agreement before we can work with your pull request (review and merge), regardless that the original contributor is already our official contributor.
It is a one-time-only action (you can than create as many pull requests as you like with it) and it takes usually several weeks, until it is officially listed on the page linked above, which is the relevant source for us.
Regards,
Adam

@AdamLindenthal
Copy link
Member

Oh sorry for the comment above, this should have belonged to another pull request (and of course with someone other's name)!

@Stephan202
Copy link
Contributor Author

No problem ;)

mgajdos added a commit that referenced this pull request Feb 3, 2015
Fix various issues with JarFileScanner
@mgajdos mgajdos merged commit 66d1dc0 into javaee:master Feb 3, 2015
@mgajdos
Copy link
Contributor

mgajdos commented Feb 3, 2015

@Stephan202 Thank you for you contribution.

@Stephan202 Stephan202 deleted the jar-file-scanner-fix branch February 3, 2015 13:44
@shamoh shamoh added this to the 2.16 milestone Feb 13, 2015
@cguillot
Copy link

Hi,
I tried, but, it can't work...

The package name provided to the JarFileScanner constructor is extracted from URLs returned by ResourcesProvider.getResources() default implementation which is a ClassLoader.getResources("com/my/package") (see org.glassfish.jersey.server.internal.scanning.PackageNamesScanner.init() for package name building).
So whatever the package name, it will come back with its trailing slash.

I think it worths a JIRA ?!

As a fix, for the recursive part, as the parent is a full qualified path (with the trailing slash, e.g.: com/my/package/ ) if it's found at the beginning of the JarEntry name, this entry is a sub resource of this package. If some packages are provided without this trailing slash, it's an error i guess ? So maybe we just have a check the presence of the trailing slash when building a JarFileScanner.

@cguillot
Copy link

Or maybe it is the philosophy of the JarFileScanner to be able to handle full packages path or just a part of the path !
The javadoc just says:

  • parent - JAR file entry prefix.

@Stephan202
Copy link
Contributor Author

Clear, thanks for getting back to this. Somewhere this week I'll try to open a PR with a fix that allows the delimiting slash to either be implicit (as the code supports now) or explicit (your use case).

@stromnet
Copy link

Hi,

this may or may not be related (most likely not) to your changes, but it is related to resource scanning. I've found some issues with OSGI scanning. For more details, see https://java.net/jira/browse/JERSEY-2788 (specifically the third comment where I've pinpointed the issue).

Regards
Johan

@Stephan202
Copy link
Contributor Author

@stromnet: indeed, I do not think that issue is related to this PR.

@cguillot: I filed #145 to address your concerns :).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants