Skip to content

New Feature: Change Artifact Owner(ship)#2731

Merged
EricWittmann merged 8 commits intoApicurio:mainfrom
EricWittmann:feat/change-ownership
Aug 29, 2022
Merged

New Feature: Change Artifact Owner(ship)#2731
EricWittmann merged 8 commits intoApicurio:mainfrom
EricWittmann:feat/change-ownership

Conversation

@EricWittmann
Copy link
Member

@EricWittmann EricWittmann commented Aug 26, 2022

Overview

This PR adds support for changing the owner of an Artifact. Support is only added to the REST API. A future PR will add support via the UI and another for the CLI.

There are two new REST API operations (for one new endpoint):

Get Artifact Owner

image

Update Artifact Owner

image

Note that the Update Artifact Owner operation is unique in that it can be called successfully only by the current owner of the artifact or by a user with the Admin role.

@apicurio-bot
Copy link

apicurio-bot bot commented Aug 26, 2022

Thank you for creating a pull request!

Pinging @carlesarnal to respond or triage.

@apicurio-bot apicurio-bot bot added area/rest Issues related to one of the REST APIs. area/sdk/java Issues related specifically to the Java SDK. area/storage Issues related to the storage layer (SQL or Kafka). labels Aug 26, 2022
@EricWittmann EricWittmann marked this pull request as ready for review August 29, 2022 11:48
@EricWittmann
Copy link
Member Author

Note that some changes to the access controllers was necessary because a new type of access was added: "Owner or Admin"

This new level of access just says that an operation can be performed either by the owner of the artifact or by an Admin. This logic required some refactoring of the access controllers.

Copy link
Member

@carlesarnal carlesarnal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with the small comment about the super vs delegate.

@Override
public void updateArtifactOwner(String groupId, String artifactId, ArtifactOwnerDto owner) throws ArtifactNotFoundException, RegistryStorageException {
super.updateArtifactOwner(groupId, artifactId, owner);
//TODO consider a change ownership event
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the delegate should be used here instead of super? btw, I agree, adding an event here makes sense :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably makes sense to just remove any of the methods that do nothing but call the delegate, since the parent class is already doing that. Usually when you extend a class in this way that's what you do, so that the subclass only overrides the functionality it needs. But I've made the change you suggested instead because I don't feel very strongly about it and using the delegate directly is what we're doing everywhere else. So +1 for consistency.

@EricWittmann EricWittmann merged commit f71715f into Apicurio:main Aug 29, 2022
@EricWittmann EricWittmann deleted the feat/change-ownership branch August 29, 2022 16:21
@EricWittmann
Copy link
Member Author

@smccarthy-ie FYI - this is part of the Change Owner functionality we've discussed recently.

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

Labels

area/rest Issues related to one of the REST APIs. area/sdk/java Issues related specifically to the Java SDK. area/storage Issues related to the storage layer (SQL or Kafka).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants