uranusjr commented on a change in pull request #18757:
URL: https://github.com/apache/airflow/pull/18757#discussion_r723091557
##########
File path: airflow/api_connexion/endpoints/user_endpoint.py
##########
@@ -124,9 +124,21 @@ def patch_user(username, update_mask=None):
if user is None:
detail = f"The User with username `{username}` was not found"
raise NotFound(title="User not found", detail=detail)
-
- # Get fields to update. 'username' is always excluded (and it's an error to
- # include it in update_maek).
+ # Check unique username
+ new_username = data.get('username')
+ if new_username:
+ usr = security_manager.find_user(username=new_username)
+ if usr and usr != user:
+ raise AlreadyExists(detail=f"The username `{new_username}` already
exists")
+
+ # Check unique email
+ email = data.get('email')
+ if email:
+ usr = security_manager.find_user(email=email)
+ if usr and usr != user:
+ raise AlreadyExists(detail=f"The email `{email}` already exists")
Review comment:
I think one important consideration is that those fields are pre-filled
in the web UI (I think by Flask-Appbuilder?) so setting them as required
imposes no cost to the user, but that's not the case for the API. The PATCH
verb is designed to allow the user to not send fields they don't want to
modify, so requiring fields is kind of against the spirit IMO, especially for
`email` (`username` is already needed to access the endpoint anyway so
requiring it in the payload is only unnecessary duplication and slightly less
problematic).
But even if we decide to make these fields required, they still need to have
`minLength: 1` IMO. It's very non-obvious that setting those fields to an empty
string implies not changing them. The expected behaviour for providing an empty
string in a field is setting that field to an empty string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
