-
Notifications
You must be signed in to change notification settings - Fork 10
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
Delegate URI to URL conversion to ArtifactStore. #139
Delegate URI to URL conversion to ArtifactStore. #139
Conversation
Hi Raul, We have a set of classes that will turn URIs into URLs for different archives. (here: https://github.com/opencadc/caom2/tree/master/caom2-artifact-resolvers/src/main/java/ca/nrc/cadc/caom2/artifact/resolvers) It would be nice if the download tool could make use of these and saving each implementor have to write essentially the same code. What's missing is the logic to instantiate the correct class for the URI, a resolver factory essentially: What would you think of changing this pull request into that sort of approach? When new types of resolving are required, a new resolver class is created and then added to the factory. (The 'cadc' one is not very well named: it's AdResolver.java, named after our storage implementation.) |
given the artifact URI. New AbstractArtifactStore implementing a generic toUrl(artifactUri) method using the ArtifactStoreFactory.
Hi Brian, I have created an StorageResolverFactory to determine the proper storage resolver based on the artifact URI. I have also created an AbstractArtifactStore with a general implementation of toUrl() method, so it can be reused by specific implementations of ArtifactStore. Please, let me know if this is in line with your suggestion. |
Hi Raul, Getting closer... The ArtifactStore interface is there to be the interface to the place where the file is saved (the destination), not the interface to the source of the file. So, there should be no changes to the ArtifactStore interface at all. DownloadArtifactFiles.java should just use the StorageResolverFactory directly as it is dealing with resolving the source of the download at that point. (This means that there's no need for the AbstractArtifactStore class now...) Could you please move StorageResolverFactory to a new pull request in opencadc/caom2, in the caom2-artifact-resolvers module? It should be near the actual resolvers. As for the implementation of StorageResolverFactory, there is one that exists here already that you could probably just copy and make some minor alterations: It uses a properties file to specify which class support which URI scheme. This would be more flexible I'd think. But, you'd still have to create a new resolver class for a new scheme, so I'm not sure how much value it adds... Also, could you be sure to include our file header blurb (licence info) at the top of new Java files? Many thanks for your work on this! |
Pull request created in caom2 project: Closing this to replace this pull request by the new one: #140 |
In order to delegate URI to URL conversion to specific implementations of ArtifactStore, the interface has been modified.