[#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?
There was a problem hiding this comment.
I think you make things complicated, if it is the name conflict, the old name and new name should be the same, you can use the name passed in. Both getting the name from TagChange and Exception are too complex and not necessary just for getting a name.
There was a problem hiding this comment.
But the conflict old name in updateTag step is actually come from TagChange?
And WDYM passed in name here, I don't get it.
There was a problem hiding this comment.
If it is the EntityAlreadyExistsException, which could only happen when the new name and the old name are the same, so you don't have to get the new name for TableChange, you can use the old name from the method, which is the same.
There was a problem hiding this comment.
What you need to change it to change the exception only, no need to change the message.
|
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