Community
Participate
Working Groups
Currently we have some complex code patterns for clients that create repositories. We should revisit the API to see if we can come up with an API that allows for simplification of these code patterns.
John, we need to change something here. See bug 261731 where we waffled back and forth on the order of create/load. See also bug 264891. The main problem I have is the Extension Location repository, loading it can end up generating metadata on artifacts that exist at that location.. In the case of publishing and mirroring this is both a huge and unnecessary performance hit, as well as potentially messing up the publish/mirroring we are about to attempt. The only idea I have right now would be to add a MetadataRepositoryFactory.isModifiable(URI location) This would return true if a call to load with that location would return a modifiable repository. Then we add a I*RepositoryManager.load(URI, boolean modifiableRequired, IProgressMonitor) This will then use the factor.isModifiable to ensure it only loads a repo that will be modifiable. Also, since this pattern of load and create is common, perhaps we can add a getModifiableRepository(URI, IProgressMonitor) which would do a load with modifiable required, and if that doesn't return something it would do a create.
Created attachment 125957 [details] patch Patch adds: IRepositoryManager#REPOSITORY_MODIFIABLE IArtifactRepositoryManager#loadRepository(URI, int flags, IProgressMonitor) IMetadataRepositoryManager#loadRepository(URI, int flags, IProgressMonitor) AbstractRepositoryManager#loadRepository then passes the flags through AbstractRepositoryManager#factoryLoad. Both ArtifactRepositoryFactory and MetadataRepositoryFactory are changed to #load(URI, int flags, IProgressMonitor). Subclasses of *RepositoryFactory are changed to fail fast and return null if IRepositoryManager.REPOSITORY_MODIFIABLE is set but we would get a non-modifiable repo. AbstractRepositoryManager#loadRepository(URI, suffix, type, flags, monitor) is changed to keep trying providers if one returns null.
I really like this approach and I think it addresses Bug #261731. The only problem I see with this approach is that those who provide their own repositories may not be aware if new flags are added. The alternative is to add methods to the factory interfaces (such as isModifyable()), and have the manager handle the flags. (Note: my suggestion will fail to be binary compatible as we move forward).
Direction looks good. There are some minor things to fix up in the javadoc, but you can go ahead and release and we can tweak from there. Re comment #3 - I think any flag can at best be a "hint" since you are correct that older repositories won't understand new flags.
I have released this patch with a few minor changes: Some improved javadoc on IMetadataRepositoryManager, IArtifactRepositoryManager, as well as the *RepositoryFactory classes. I changed IRepositoryManager#REPOSITORY_MODIFIABLE to REPOSITORY_HINT_MODIFIABLE I will leave this bug open for now, we should decide if we want a single entry point for the common pattern of load/create.
For the record, existing calls to IArtifactRepository and IMetadataRepository loadRepository(URI, IProgressMonitor) are deprecated and replaced with loadRepository(URI, int, IProgressMonitor) clients will currently want to switch to one of loadRepository(uri, 0, null) loadRepository(uri, IRepositoryManager.REPOSITORY_HINT_MODIFIABLE, null) depending whether or not they want a modifiable repo.
Created attachment 126177 [details] More javadoc and undeprecate This patch polishes the javadoc a bit. Also, it undeprecates the old methods. In reviewing, I noticed there are literally *hundreds* of references to these load methods, so having a convenience method for the default "no hints" case seems reasonable. Apart from the publisher/generator, most clients don't care if the repository is writable.
Latest patch released in HEAD.
I just noticed that testFeatureSiteReferences test case fails. It fails at 1.0, when a referenced update site is checked for its existence in the repo manager. Any chance this fix could have caused that? (This was passing yesterday).
(In reply to comment #9) > I just noticed that testFeatureSiteReferences test case fails. It fails at 1.0, > when a referenced update site is checked for its existence in the repo manager. > Any chance this fix could have caused that? (This was passing yesterday). > I don't think the failing test is caused by this fix. I have opened bug 265528 to track the failing test problem.
