You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@archiva.apache.org by Brett Porter <br...@apache.org> on 2009/10/13 14:49:17 UTC
Re: svn commit: r824677 - in /archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src: main/java/org/apache/maven/archiva/webdav/ test/java/org/apache/maven/archiva/webdav/ test/resources/
On 13/10/2009, at 9:36 PM, oching@apache.org 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?
> // 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
> @@ -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?
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?
Cheers,
Brett
Re: svn commit: r824677 - in /archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src:
main/java/org/apache/maven/archiva/webdav/ test/java/org/apache/maven/archiva/webdav/
test/resources/
Posted by Deng Ching <oc...@apache.org>.
On Wed, Oct 14, 2009 at 11:40 PM, Brett Porter <br...@apache.org> wrote:
>
>
>>> // 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?
ok, will move this one over..
>
>
>
>>
>>>
>>> @@ -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?
Ok, I get it now :) I think we still need to check if the version is a
SNAPSHOT or not though, and block deployment if it is a released artifact or
an invalid artifact (LayoutException is thrown) as long as it already exists
in the repository. We're also using the artifact reference to check if it is
in the repository so it's not just the version we're using. I'll see if
there's another way to do this.
Thanks,
Deng
Re: svn commit: r824677 - in /archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src: main/java/org/apache/maven/archiva/webdav/ test/java/org/apache/maven/archiva/webdav/ test/resources/
Posted by Brett Porter <br...@apache.org>.
On 14/10/2009, at 7:03 PM, Deng Ching wrote:
> Hi Brett,
>
> On Tue, Oct 13, 2009 at 8:49 PM, Brett Porter <br...@apache.org>
> wrote:
>
>>
>> On 13/10/2009, at 9:36 PM, oching@apache.org 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
Re: svn commit: r824677 - in /archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src:
main/java/org/apache/maven/archiva/webdav/ test/java/org/apache/maven/archiva/webdav/
test/resources/
Posted by Deng Ching <oc...@apache.org>.
Hi Brett,
On Tue, Oct 13, 2009 at 8:49 PM, Brett Porter <br...@apache.org> wrote:
>
> On 13/10/2009, at 9:36 PM, oching@apache.org 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
>
>
> @@ -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?
> 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
> Cheers,
> Brett
>
>