Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PAYARA-1840 RAR detector now checks for all JCA annotations #1790

Merged
merged 3 commits into from Sep 8, 2017

Conversation

Cousjava
Copy link
Contributor

All archives are now checked for annotations, regardless of type

All archives are now checked for annotations, regardless of type
@Cousjava Cousjava added this to the Payara 173 milestone Jul 24, 2017
@Cousjava Cousjava requested a review from MattGill98 July 24, 2017 13:25
@Cousjava
Copy link
Contributor Author

jenkins test please

@payara-ci
Copy link
Contributor

Quick build and test passed!

@@ -151,8 +152,7 @@ public void scanArchive(ReadableArchive archive) {
} finally {
is.close();
}
} else if (entryName.endsWith(".jar") &&
entryName.indexOf('/') == -1) {
} else if (!entryName.contains("/")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why has this changed? It looks like it may produce side effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because otherwise it will not scan non-jars for annotations. The code below it is in a try statement anyway so any problems will be caught.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side effect when calling jarSubArchive.entries().

com.sun.enterprise.deployment.deploy.shared.InputJarArchive#entries(boolean) does not throw an IOException but a RuntimeException. The effect is the exception is not catch by the inner try-catch bloc but the outer one, skipping all other entries in the archive.

javax.resource.spi.Connector.class};
javax.resource.spi.Connector.class,
javax.resource.spi.ConfigProperty.class,
javax.resource.spi.ConnectionDefinition.class,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct as other applications can have these annotations and aren't rar files.

You need to see which ones are rar only and which ones can be used by clients of a rar

@Cousjava Cousjava modified the milestones: Payara 174, Payara 173 Jul 31, 2017
@Cousjava
Copy link
Contributor Author

Cousjava commented Sep 7, 2017

jenkins test please

private static final Class[] connectorAnnotations = new Class[]{
javax.resource.spi.Connector.class};
javax.resource.spi.Connector.class,
javax.resource.spi.ConfigProperty.class,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ConfigProperty is used in WARs which use Resource Adapters, I'd swap it for @Activation

@payara-ci
Copy link
Contributor

Quick build and test passed!

@Cousjava
Copy link
Contributor Author

Cousjava commented Sep 7, 2017

jenkins test please

@payara-ci
Copy link
Contributor

Quick build and test passed!

@Cousjava Cousjava merged commit b875db7 into payara:master Sep 8, 2017
michaelranaldo pushed a commit to michaelranaldo/Payara that referenced this pull request Sep 12, 2017
)

* PAYARA-1840 RAR detector now checks for all JCA annotations

All archives are now checked for annotations, regardless of type

* removed some annotations from possible ones to detect

* changed annotations
Copy link
Contributor

@maxencelaurent maxencelaurent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In GenericAnnotationDetector#scanArchive,, trying to read any non-.class file as zip archive has side effect

@@ -151,8 +152,7 @@ public void scanArchive(ReadableArchive archive) {
} finally {
is.close();
}
} else if (entryName.endsWith(".jar") &&
entryName.indexOf('/') == -1) {
} else if (!entryName.contains("/")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side effect when calling jarSubArchive.entries().

com.sun.enterprise.deployment.deploy.shared.InputJarArchive#entries(boolean) does not throw an IOException but a RuntimeException. The effect is the exception is not catch by the inner try-catch bloc but the outer one, skipping all other entries in the archive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants