[#6254]Improve: Rename tag to a name already exists in the current metalake, it should return 409 instead of 500 #6298
Conversation
…ent metalake, it should return 409 instead of 500 ### What changes were proposed in this pull request? 1. Rename tag to a name already exists in the current metalake, it should return 409 instead of 500 2. The error message should show the correct conflict name ### Why are the changes needed? ### Fix: apache#6254 ### Does this PR introduce any user-facing change? No ### How was this patch tested? ut
| private String getNewName(TagChange... changes) { | ||
| for (TagChange change : changes) { | ||
| if (change instanceof TagChange.RenameTag) { | ||
| return ((TagChange.RenameTag) change).getNewName(); | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
You can get the new name from the updated tag entity, no need to do it here. Also, it is not proper to return null, which will be weird for the printed log.
There was a problem hiding this comment.
The updated Tag entity is being constructed within a deeply nested callback function, making it challenging to access (since the error occurs, the updated Tag entity cannot be retrieved as a return value). Alternatively, I could include the error name in the exception thrown by the TagMetaService.updateTag() method.
So I believe this alternative approach is simpler and more straightforward for retrieving the name.
In this case, if a TagAlreadyExistsException is triggered, the method will never return null. However, instead of returning null in cases where the method might be misused, it might be better to throw an error to explicitly handle such situations.
Which one do you prefer?
Get the name from the Exception or TagChange?
|
Beside the code change here, I think also update the related openapi doc. |
What changes were proposed in this pull request?
Why are the changes needed?
Fix: #6254
Does this PR introduce any user-facing change?
No
How was this patch tested?
ut