Skip to content
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

Fix Owner Validation #3593

Merged
merged 1 commit into from
Aug 29, 2023
Merged

Conversation

renjingxiao
Copy link
Contributor

Hi @EricWittmann and @carlesarnal ,

In this PR I have added checks for the owner to ensure it's neither null or an empty string. MissingRequiredParameterException will be thrown if the check fails.

It will close #3476 .

Thank you for reviewing.

@@ -259,7 +259,13 @@ public void updateArtifactOwner(String groupId, String artifactId, ArtifactOwner
requireParameter("groupId", groupId);
requireParameter("artifactId", artifactId);

ArtifactOwnerDto dto = new ArtifactOwnerDto(data.getOwner());
String owner = data.getOwner();
requireParameter("owner", owner);
Copy link
Member

Choose a reason for hiding this comment

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

Is calling the requireParameter method not sufficient for this purpose?

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.

Please address the feedback or reply to the comment.

@renjingxiao
Copy link
Contributor Author

Hi @carlesarnal , thank you for the review. I thought requireParameter only checks for null, so I added lines 264-266 to check if it's an empty value. If they are redundant I'll remove them.

@renjingxiao
Copy link
Contributor Author

Hi @carlesarnal ,
I have added the unit tests. Could you please have a look?
Thank you

Copy link
Member

@EricWittmann EricWittmann left a comment

Choose a reason for hiding this comment

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

Looks perfect, thanks @renjingxiao

@EricWittmann EricWittmann merged commit 2755e83 into Apicurio:main Aug 29, 2023
@renjingxiao renjingxiao deleted the non-empty-owner branch August 30, 2023 14:32
@carlesarnal carlesarnal added the backport-2.x Backport fix to Registry v2 label Nov 3, 2023
@carlesarnal carlesarnal self-assigned this Nov 7, 2023
carlesarnal pushed a commit to carlesarnal/apicurio-registry that referenced this pull request Nov 13, 2023
carlesarnal added a commit that referenced this pull request Nov 13, 2023
* Add null checks on  role mapping (#3800)

* Add null checks on  role mapping

* Add check for Role and Unit Test

* Fixed a regression in the Settings page search box (#3798)

* modified state change checks in ArtifactStateExt and AbstractSqlRegistryStorage (#3637)

* Incorrect default value avro serialization (#3633)

* Fix default value converter

* Add test for default byte type value in converter test

* Correct comment

* added unit tests (#3593)

* Bugfix: prase decimal's default

---------

Co-authored-by: Amoncy <[email protected]>
Co-authored-by: Eric Wittmann <[email protected]>
Co-authored-by: Jean Xiao <[email protected]>
Co-authored-by: Anshu Anna Moncy <[email protected]>
Co-authored-by: j2gg0s <[email protected]>
carlesarnal added a commit to carlesarnal/apicurio-registry that referenced this pull request Nov 13, 2023
* Add null checks on  role mapping (Apicurio#3800)

* Add null checks on  role mapping

* Add check for Role and Unit Test

* Fixed a regression in the Settings page search box (Apicurio#3798)

* modified state change checks in ArtifactStateExt and AbstractSqlRegistryStorage (Apicurio#3637)

* Incorrect default value avro serialization (Apicurio#3633)

* Fix default value converter

* Add test for default byte type value in converter test

* Correct comment

* added unit tests (Apicurio#3593)

* Bugfix: prase decimal's default

---------

Co-authored-by: Amoncy <[email protected]>
Co-authored-by: Eric Wittmann <[email protected]>
Co-authored-by: Jean Xiao <[email protected]>
Co-authored-by: Anshu Anna Moncy <[email protected]>
Co-authored-by: j2gg0s <[email protected]>
carlesarnal added a commit that referenced this pull request Nov 13, 2023
* Add null checks on  role mapping (#3800)

* Add null checks on  role mapping

* Add check for Role and Unit Test

* Fixed a regression in the Settings page search box (#3798)

* modified state change checks in ArtifactStateExt and AbstractSqlRegistryStorage (#3637)

* Incorrect default value avro serialization (#3633)

* Fix default value converter

* Add test for default byte type value in converter test

* Correct comment

* added unit tests (#3593)

* Bugfix: prase decimal's default

---------

Co-authored-by: Amoncy <[email protected]>
Co-authored-by: Eric Wittmann <[email protected]>
Co-authored-by: Jean Xiao <[email protected]>
Co-authored-by: Anshu Anna Moncy <[email protected]>
Co-authored-by: j2gg0s <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rest-api backport-2.x Backport fix to Registry v2
Projects
No open projects
Status: Released
Development

Successfully merging this pull request may close these issues.

Setting an empty "owner" is allowed in the REST API
3 participants