SRAMP-531 SRAMP-580 Server-side auto-detection of artifact types, replac... #511

Merged
merged 1 commit into from Dec 1, 2014

Conversation

Projects
None yet
3 participants
@brmeyer
Contributor

brmeyer commented Nov 26, 2014

...ed "expander" concepts

@brmeyer

This comment has been minimized.

Show comment
Hide comment
@brmeyer

brmeyer Nov 26, 2014

Contributor

@objectiser & @EricWittmann, this thing is huge, but I'd really appreciate a review of the contract and its basic use. I'll point out specific areas with comments. More eyes on this would be great, since it has a lot of implications.

Note that I did not treat this like the old Deriver and deprecate it. Instead, the old expander contracts (atom.archive.expand.*) are blown away. If you disagree with that, feel free to say so.

Contributor

brmeyer commented Nov 26, 2014

@objectiser & @EricWittmann, this thing is huge, but I'd really appreciate a review of the contract and its basic use. I'll point out specific areas with comments. More eyes on this would be great, since it has a lot of implications.

Note that I did not treat this like the old Deriver and deprecate it. Instead, the old expander contracts (atom.archive.expand.*) are blown away. If you disagree with that, feel free to say so.

@@ -0,0 +1,90 @@
+/*

This comment has been minimized.

@brmeyer

brmeyer Nov 26, 2014

Contributor

@objectiser & @EricWittmann, this is the main contract. Let me know if the javadocs make sense.

@brmeyer

brmeyer Nov 26, 2014

Contributor

@objectiser & @EricWittmann, this is the main contract. Let me know if the javadocs make sense.

@@ -0,0 +1,97 @@
+/*

This comment has been minimized.

@brmeyer

brmeyer Nov 27, 2014

Contributor

@objectiser & @EricWittmann, this is a decent example of what's possible. It checks for the existence of switchyard.xml to "detect" a SY app archive. Expansion is...expanded...to allow Java classes to be uploaded, but only if 1.) switchyard.xml references them or 2.) there is no switchyard.xml (see the note).

@brmeyer

brmeyer Nov 27, 2014

Contributor

@objectiser & @EricWittmann, this is a decent example of what's possible. It checks for the existence of switchyard.xml to "detect" a SY app archive. Expansion is...expanded...to allow Java classes to be uploaded, but only if 1.) switchyard.xml references them or 2.) there is no switchyard.xml (see the note).

@@ -0,0 +1,129 @@
+/*

This comment has been minimized.

@brmeyer

brmeyer Nov 27, 2014

Contributor

@objectiser & @EricWittmann, another piece of the contract. It's mainly to allow on-demand InputStreams, as well as additional File info.

@brmeyer

brmeyer Nov 27, 2014

Contributor

@objectiser & @EricWittmann, another piece of the contract. It's mainly to allow on-demand InputStreams, as well as additional File info.

@@ -0,0 +1,200 @@
+/*

This comment has been minimized.

@brmeyer

brmeyer Nov 27, 2014

Contributor

@objectiser & @EricWittmann, this is the other piece. It provides archive-specific context for the type detectors. Some semblance of this existed, but the specifics are pretty new.

@brmeyer

brmeyer Nov 27, 2014

Contributor

@objectiser & @EricWittmann, this is the other piece. It provides archive-specific context for the type detectors. Some semblance of this existed, but the specifics are pretty new.

+ // NOTE: By default, classes are not allowed by AbstractArtifactTypeDetector#allowExpansionFromArchive.
+ // However, in case another detector allows them under certain circumstances
+ // (see SwitchYardArtifactTypeDetector), handle the detection here.
+ if (content.getFilename().endsWith(".class")) {

This comment has been minimized.

@brmeyer

brmeyer Nov 27, 2014

Contributor

@objectiser & @EricWittmann, this is one discussion point. Technically, this sets S-RAMP up to expand all discovered Java classes. However, at the moment, only SY is the one that's actually allowing it. This is the same as it was before. I can't think of many reasons why expanded all of the classes would actually be desirable, except maybe if it's referenced by beans.xml or a WSDL. Thoughts?

@brmeyer

brmeyer Nov 27, 2014

Contributor

@objectiser & @EricWittmann, this is one discussion point. Technically, this sets S-RAMP up to expand all discovered Java classes. However, at the moment, only SY is the one that's actually allowing it. This is the same as it was before. I can't think of many reasons why expanded all of the classes would actually be desirable, except maybe if it's referenced by beans.xml or a WSDL. Thoughts?

This comment has been minimized.

@objectiser

objectiser Nov 27, 2014

Contributor

Yes, if referenced, then could be useful - otherwise no.

@objectiser

objectiser Nov 27, 2014

Contributor

Yes, if referenced, then could be useful - otherwise no.

This comment has been minimized.

@EricWittmann

EricWittmann Dec 1, 2014

Contributor

Agreed, although it can be hard to know that at the time of expansion. If the artifact is a JAR which contains interfaces later used by downstream SY applications, for example. :(

@EricWittmann

EricWittmann Dec 1, 2014

Contributor

Agreed, although it can be hard to know that at the time of expansion. If the artifact is a JAR which contains interfaces later used by downstream SY applications, for example. :(

This comment has been minimized.

- if (artifactType.isDerived()) {
- throw new DerivedArtifactCreateException(artifactType.getArtifactType());
- }
+ /**

This comment has been minimized.

@brmeyer

brmeyer Nov 27, 2014

Contributor

This all required a new endpoint that takes a filename and InputStream, but no model/type. Definitely non-spec, but it's isolated to /s-ramp/autodetect. Methods were added to the clients to reflect that.

@brmeyer

brmeyer Nov 27, 2014

Contributor

This all required a new endpoint that takes a filename and InputStream, but no model/type. Definitely non-spec, but it's isolated to /s-ramp/autodetect. Methods were added to the clients to reflect that.

This comment has been minimized.

@objectiser

objectiser Nov 27, 2014

Contributor

Possibly as non-standard should the url reflect that, e.g. /s-ramp/ext/autodetect?

@objectiser

objectiser Nov 27, 2014

Contributor

Possibly as non-standard should the url reflect that, e.g. /s-ramp/ext/autodetect?

This comment has been minimized.

@brmeyer

brmeyer Dec 1, 2014

Contributor

+1 -- prefixing them all with /ext is a good idea.

@brmeyer

brmeyer Dec 1, 2014

Contributor

+1 -- prefixing them all with /ext is a good idea.

@brmeyer

This comment has been minimized.

Show comment
Hide comment
@brmeyer

brmeyer Nov 27, 2014

Contributor

Alright, that should be it. Note that all unit tests pass, and the integration tests pass on the dev server. But I'm having a few odd, intermittent Teiid/SY-related failures on Tomcat/EAP that I'll chase down next week.

Contributor

brmeyer commented Nov 27, 2014

Alright, that should be it. Note that all unit tests pass, and the integration tests pass on the dev server. But I'm having a few odd, intermittent Teiid/SY-related failures on Tomcat/EAP that I'll chase down next week.

+ // "SwitchYardApplication" type, but it does not include a switchyard.xml (ie, no SwitchYardAppIndex),
+ // we need to expand all classes by default! This is in order to support multi-module SY applications
+ // where common classes are in a separate jar.
+ return true;

This comment has been minimized.

@objectiser

objectiser Nov 27, 2014

Contributor

This is probably fine for now, but if an interface is being used for a service, then switchyard annotates it - so instead of just expanding all, might be worth checking for classes that have the relevant annotation (assuming it is a runtime annotation).

@objectiser

objectiser Nov 27, 2014

Contributor

This is probably fine for now, but if an interface is being used for a service, then switchyard annotates it - so instead of just expanding all, might be worth checking for classes that have the relevant annotation (assuming it is a runtime annotation).

@objectiser

This comment has been minimized.

Show comment
Hide comment
@objectiser

objectiser Nov 27, 2014

Contributor

Looks good to me - good work!

Contributor

objectiser commented Nov 27, 2014

Looks good to me - good work!

brmeyer added a commit that referenced this pull request Dec 1, 2014

Merge pull request #511 from brmeyer/SRAMP-531
SRAMP-531 SRAMP-580 Server-side auto-detection of artifact types, replac...

@brmeyer brmeyer merged commit f8711a8 into ArtificerRepo:master Dec 1, 2014

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