SRAMP-609, SRAMP-547, SRAMP-466: ArtifactBuilder #508

Merged
merged 5 commits into from Oct 30, 2014

Conversation

Projects
None yet
2 participants
@brmeyer
Contributor

brmeyer commented Oct 29, 2014

Really, SRAMP-609 implicitly corrected the other issues. However, keeping the commits separate for readability. If this ever needs backported, all the commits should be taken in one shot -- don't attempt to separate them.

+ */
+public class XsdDocumentArtifactBuilder extends AbstractXmlArtifactBuilder {
+
+ protected IndexedArtifactCollection derivedArtifacts = new IndexedArtifactCollection();

This comment has been minimized.

@EricWittmann

EricWittmann Oct 29, 2014

Contributor

This shadows a field in AbstractArtifactBuilder. I found that confusing when reading through the code. Perhaps this should be pushed down to AbstractArtifactBuilder?

@EricWittmann

EricWittmann Oct 29, 2014

Contributor

This shadows a field in AbstractArtifactBuilder. I found that confusing when reading through the code. Perhaps this should be pushed down to AbstractArtifactBuilder?

This comment has been minimized.

@brmeyer

brmeyer Oct 30, 2014

Contributor

Ya, this is an area that could be improved later. The tough part is that AbstractArtifactBuilder needs to provide a default collection for the derivedArtifacts, but subclasses can override that with something more specific. For XSD and WSDLs, that's IndexedArtifactCollection. The only other one is WebXmlArtifactCollection, which is simply part of a demo. For all others, the simple ArrayList in AbstractArtifactBuilder is used.

That could be made more generic to have one single type of IndexedArtifactCollection that could potentially cover all cases...

@brmeyer

brmeyer Oct 30, 2014

Contributor

Ya, this is an area that could be improved later. The tough part is that AbstractArtifactBuilder needs to provide a default collection for the derivedArtifacts, but subclasses can override that with something more specific. For XSD and WSDLs, that's IndexedArtifactCollection. The only other one is WebXmlArtifactCollection, which is simply part of a demo. For all others, the simple ArrayList in AbstractArtifactBuilder is used.

That could be made more generic to have one single type of IndexedArtifactCollection that could potentially cover all cases...

+ * @return ArtifactBuilder
+ * @throws IOException
+ */
+ public ArtifactBuilder buildArtifacts(BaseArtifactType primaryArtifact, byte[] contentBytes) throws IOException;

This comment has been minimized.

@EricWittmann

EricWittmann Oct 29, 2014

Contributor

Should content be an inputstream instead of bytes? Or perhaps some other fancy content object that can provide access to the content in a number of ways. I'd be concerned what this will do for large files.

@EricWittmann

EricWittmann Oct 29, 2014

Contributor

Should content be an inputstream instead of bytes? Or perhaps some other fancy content object that can provide access to the content in a number of ways. I'd be concerned what this will do for large files.

This comment has been minimized.

@brmeyer

brmeyer Oct 30, 2014

Contributor

Good point. The main problem is that building, persisting, etc. needs to read the stream more than once, so we can't simply rely on the initial InputStream given to S-RAMP. So the thought was that by providing the byte[], you could build a ByteArrayInputStream on-demand. But for large files, that's a great point. Any ideas?

@brmeyer

brmeyer Oct 30, 2014

Contributor

Good point. The main problem is that building, persisting, etc. needs to read the stream more than once, so we can't simply rely on the initial InputStream given to S-RAMP. So the thought was that by providing the byte[], you could build a ByteArrayInputStream on-demand. But for large files, that's a great point. Any ideas?

@EricWittmann

This comment has been minimized.

Show comment
Hide comment
@EricWittmann

EricWittmann Oct 29, 2014

Contributor

OK done with a high level overview. This looks good to me. Very thorough!

Contributor

EricWittmann commented Oct 29, 2014

OK done with a high level overview. This looks good to me. Very thorough!

brmeyer added a commit that referenced this pull request Oct 30, 2014

Merge pull request #508 from brmeyer/SRAMP-547
SRAMP-609, SRAMP-547, SRAMP-466: ArtifactBuilder

@brmeyer brmeyer merged commit e7ba7b3 into ArtificerRepo:master Oct 30, 2014

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