On 14/10/2009, at 7:03 PM, Deng Ching wrote:
> Hi Brett,
>
> On Tue, Oct 13, 2009 at 8:49 PM, Brett Porter <
brett@...>
> wrote:
>
>>
>> On 13/10/2009, at 9:36 PM,
oching@... wrote:
>>
>> - resource =
>>> - processRepositoryGroup( request,
>>> archivaLocator,
>>> repoGroupConfig.getRepositories(),
>>> - activePrincipal,
>>> resourcesInAbsolutePath );
>>> + try
>>> + {
>>> + resource =
>>> + processRepositoryGroup( request,
>>> archivaLocator,
>>> repoGroupConfig.getRepositories(),
>>> + activePrincipal,
>>> resourcesInAbsolutePath );
>>> + }
>>> + catch ( ReleaseArtifactAlreadyExistsException e )
>>> + {
>>> + throw new DavException(
>>> HttpServletResponse.SC_CONFLICT );
>>> + }
>>>
>>
>>
>> it might make more sense just to throw this at the source and
>> eliminate the
>> exception, since the result is always the same?
>
>
> agreed :)
>
>
>>
>> // MRM-872 : merge all available metadata
>>> // merge metadata only when requested via the repo group
>>> - if ( ( repositoryRequest.isMetadata( requestedResource )
>>> || (
>>> requestedResource.endsWith( "metadata.xml.sha1" ) ||
>>> requestedResource.endsWith( "metadata.xml.md5" ) ) )
>>> - && repoGroupConfig != null )
>>> + if ( ( repositoryRequest.isMetadata( requestedResource )
>>> || (
>>> requestedResource.endsWith( "metadata.xml.sha1" ) ||
>>> requestedResource.endsWith( "metadata.xml.md5" ) ) ) &&
>>> + repoGroupConfig != null )
>>>
>>
>> Should this use "isSupportFile" like below? That will cover the two
>> metadata checksums
>
>
> .. but it will also get the other non-metadata checksum files so I
> don't
> think we can use isSupportFile(..) here
>
ok - could the check be moved to the repository request (eg,
isMetadataSupportFile), so that it is all in one spot?
>
>>
>>
>> @@ -482,6 +496,35 @@
>>>
>>> if ( request.getMethod().equals( HTTP_PUT_METHOD ) )
>>> {
>>> + String resourcePath = logicalResource.getPath();
>>> +
>>> + // check if target repo is enabled for releases
>>> + // we suppose that release-artifacts can deployed
>>> only to
>>> repos enabled for releases
>>> + if ( managedRepository.getRepository().isReleases
>>> () &&
>>> !repositoryRequest.isMetadata( resourcePath ) &&
>>> + !repositoryRequest.isSupportFile
>>> ( resourcePath ) )
>>> + {
>>> + ArtifactReference artifact = null;
>>> + try
>>> + {
>>> + artifact =
>>> managedRepository.toArtifactReference(
>>> resourcePath );
>>> + }
>>> + catch ( LayoutException e )
>>> + {
>>> + throw new DavException(
>>> HttpServletResponse.SC_BAD_REQUEST, e );
>>> + }
>>> +
>>> + if ( !VersionUtil.isSnapshot
>>> ( artifact.getVersion() )
>>> )
>>> + {
>>> + // check if artifact already exists
>>> + if ( managedRepository.hasContent
>>> ( artifact ) )
>>> + {
>>> + log.warn( "Overwriting released
>>> artifacts is
>>> not allowed." );
>>> + throw new
>>> ReleaseArtifactAlreadyExistsException( managedRepository.getId(),
>>> +
>>> "Overwriting released artifacts is not allowed." );
>>> + }
>>> + }
>>> + }
>>> +
>>>
>>
>> Is it necessarily a bad request if the reference can't be derived, or
>> should the check just be skipped?
>>
>
> I don't think this is just a check though but it's for getting the
> artifact
> object and its coordinates. Maybe we could add a fall back for
> getting the
> artifact obj & its coordinates when a LayoutException is thrown
> instead of
> immediately propagating it as a bad request error?
The check I meant was the VersionUtil bit.
Say you are trying to store /foo.txt using webdav, which is not a
valid artifact. I believe this was still allowed previously (though I
might be wrong) - but now it will be a bad request because of the
artifact path, when all it needs that for is to check if it is a
release. Does that make sense?
>
>
>> Also, given this is a point release (1.2.3), I don't think this
>> kind of
>> functionality change should be imposed on users - can we offer a
>> configuration option?
>>
>
> +1
> I've left the issue open yesterday since this functionality also
> needs to be
> applied to the web upload.
>
> Thanks,
> Deng
>
Thanks!
- Brett
