Change OpenAPI backend generator config #4376

markxiong0122 · 2024-09-19T11:26:45Z

PR Summary:

This PR changed the -g value in the npm command openapi-backend from flask-python to python to allow circular imports in our OAS to further land the implementation of OAS on features_api.

It also:

changed how OAS models were constructed (other than from_dict, we also use model_construct)
modified some test cases to align with the newly generated models
added some missed details (mainly nullable value in OAS)

jcscottiii · 2024-09-19T13:53:08Z

api/reviews_api.py

@@ -57,7 +57,7 @@ def do_get(self, **kwargs) -> dict[str, list[dict[str, Any]]]:
    # Note: We assume that anyone may view approvals.
    votes = Vote.get_votes(feature_id=feature_id, gate_id=gate_id)
    dicts = [converters.vote_value_to_json_dict(v) for v in votes]
-    return GetVotesResponse.from_dict({'votes': dicts}).to_dict()
+    return GetVotesResponse.model_construct(votes=dicts).to_dict()


Suggested change

return GetVotesResponse.model_construct(votes=dicts).to_dict()

return GetVotesResponse.from_dict({'votes': dicts}).to_dict()

model_construct takes in the expected object type for each argument. (votes should expect a variable of type List[Votes]). However, this is a special case where the existing code does this special conversion to dictionaries first. So instead, you can use the class method from_dict.

This did not work either. This indeed helps convert the dict to model and then using to_dict() to convert it back, but it seems like the serialization & deserialization during the process changed the order of stuff we put in the dict and therefore differ from the test case. How would you suggest circumventing this situation?

What error are you seeing? When I tried this suggestion earlier, the test started passing.

i got
FAIL: test_do_get__success (api.reviews_api_test.GatesAPITest.test_do_get__success) Handler retrieves all gates associated with a given feature. Traceback (most recent call last): File "/opt/homebrew/Cellar/python@3.11/3.11.8/Frameworks/Python.framework/Versions/3.11/lib/python3.11/unittest/mock.py", line 1375, in patched return func(*newargs, **newkeywargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/mark/chromium-dashboard/api/reviews_api_test.py", line 349, in test_do_get__success self.assertEqual(actual, expected) AssertionError: {'gat[126 chars]e', 'state': 1, 'assignee_emails': [], 'additi[125 chars]ne}]} != {'gat[126 chars]e', 'escalation_email': None, 'state': 1, 'req[265 chars]']}]} Diff is 674 characters long. Set self.maxDiff to None to see it. Looks like the order is still messed up. The only change I made was to undo the changes in the current commit to return GetVotesResponse.from_dict({'votes': dicts}).to_dict()

That failing test is for a different method than this one. This test should be passing now. Could you verify that?

The GatesAPITest.test_do_get__success test is for another class in the same file. I verified that GatesAPITest.test_do_get__success is failing for me too.

@mock.patch('internals.approval_defs.get_approvers') def test_do_get__success(self, mock_get_approvers): """Handler retrieves all gates associated with a given feature.""" mock_get_approvers.return_value = ['reviewer1@example.com'] self.maxDiff = None with test_app.test_request_context(self.request_path): actual = self.handler.do_get(feature_id=self.feature_id) expected = { "gates": [ { "id": 1, "feature_id": self.feature_id, "stage_id": 1, "gate_type": 1, "team_name": "API Owners", "gate_name": "Intent to Prototype", "escalation_email": None, "state": 1, # "requested_on": None, # "responded_on": None, "assignee_emails": [], # "next_action": None, "additional_review": False, 'slo_initial_response': 5, # 'slo_initial_response_took': None, # 'slo_initial_response_remaining': None, 'possible_assignee_emails': ['reviewer1@example.com'], }, ], } self.assertEqual(actual, expected)

You will see that it is not an order issue but it is removing the null values instead of defaulting to None. That would be the first thing I would look into.

Seems like if you need to add "nullable: true" to those fields I commented out in my above comment (similar to escalation_email). Then, it will work. Then you can revert the commented lines I made in the test.

pasted the wrong test.. my bad. Adding nullable: true gets the test passed. I'll modify other specs as well to see if all tests could pass. Thanks!

markxiong0122 · 2024-09-22T11:18:39Z

looks like after fixing the we we build objects and those errors, mypy is not working as expected as it detects some errors in files I didn't even modify. Any thoughts on how we could navigate this? I've fixed some but there're still some @jcscottiii thanks!

jcscottiii · 2024-09-26T14:08:10Z

Let's take a look at the errors for the non generated files:

api/reviews_api.py:60: error: Item "None" of "GetVotesResponse | None" has no attribute "to_dict"  [union-attr]
api/reviews_api.py:149: error: Item "None" of "GetGateResponse | None" has no attribute "to_dict"  [union-attr]
api/reviews_api.py:158: error: Item "None" of "PostGateRequest | None" has no attribute "assignees"  [union-attr]
api/reviews_api.py:166: error: Argument 5 to "notify_assignees" has incompatible type "list[str] | Any | None"; expected "list[str]"  [arg-type]
api/component_users.py:55: error: Item "None" of "ComponentUsersRequest | None" has no attribute "owner"  [union-attr]
api/component_users.py:64: error: Item "None" of "ComponentUsersRequest | None" has no attribute "owner"  [union-attr]
api/comments_api.py:94: error: Item "None" of "CommentsRequest | None" has no attribute "comment"  [union-attr]
api/comments_api.py:95: error: Item "None" of "CommentsRequest | None" has no attribute "post_to_thread_type"  [union-attr]
api/comments_api.py:114: error: Argument 4 to "notify_subscribers_of_new_comments" has incompatible type "str | Any | None"; expected "str"  [arg-type]
api/comments_api.py:124: error: Argument 5 to "post_comment_to_mailing_list" has incompatible type "str | Any | None"; expected "str"  [arg-type]
api/comments_api.py:131: error: Item "None" of "PatchCommentRequest | None" has no attribute "comment_id"  [union-attr]
api/comments_api.py:138: error: Item "None" of "PatchCommentRequest | None" has no attribute "is_undelete"  [union-attr]

There are two types of errors union-attr and arg-type

`union-attr` example.

In reviews_api.py, it complains about this line:

    return GetVotesResponse.from_dict({'votes': dicts}).to_dict()

This is because from_dict with this new openapi config returns Optional[Self]. So the linter is warning that we need to be careful about using the returned value.

    response = GetVotesResponse.from_dict({'votes': dicts})
    if not response:
      logging.error(f"unable to convert response from get_votes to GetVotesResponse dicts={dicts}")
      return self.abort(500)
    return response.to_dict()

This fixes that error.

You will need to do that for others.

arg-type

message: error: Argument 5 to "notify_assignees" has incompatible type "list[str] | Any | None"; expected "list[str]" [arg-type]

original code:

  def do_post(self, **kwargs) -> dict[str, str]:
    """Set the assignees for a gate."""
    user, fe, gate, feature_id, gate_id = get_user_feature_and_gate(
        self, kwargs)
    request = PostGateRequest.from_dict(self.request.json)
    assignees = request.assignees

    self.require_permissions(user, fe, gate)
    self.validate_assignees(assignees, fe, gate)
    old_assignees = gate.assignee_emails
    gate.assignee_emails = assignees
    gate.put()
    notifier_helpers.notify_assignees(
        fe, gate, user.email(), old_assignees, assignees)

    # Callers don't use the JSON response for this API call.
    return SuccessMessage(message='Done').to_dict()

This code actually has a union-attr that needs to be fixed first. Because it thinks that things could be optional, you need to verify first. Then this error goes away automatically

  def do_post(self, **kwargs) -> dict[str, str]:
    """Set the assignees for a gate."""
    user, fe, gate, feature_id, gate_id = get_user_feature_and_gate(
        self, kwargs)
+    request = PostGateRequest.from_dict(self.request.json)
+    if not request:
+      self.abort(400)
    assignees = request.assignees

    self.require_permissions(user, fe, gate)
    self.validate_assignees(assignees, fe, gate)
    old_assignees = gate.assignee_emails
    gate.assignee_emails = assignees
    gate.put()
    notifier_helpers.notify_assignees(
        fe, gate, user.email(), old_assignees, assignees)

    # Callers don't use the JSON response for this API call.
    return SuccessMessage(message='Done').to_dict()

#Other notes:

You will see that instances where I do from_dict on the request objects, I do self.abort(400). But in instances where I do from_dict while returning something from the database, I do self.abort(500)

DanielRyanSmith · 2024-09-26T14:50:24Z

framework/basehandlers.py

+from framework import csp, permissions, secrets, users, utils, xsrf
+from internals import approval_defs, notifier_helpers, user_models


Is there a reason for updating the imports like this? This does not hurt the functionality, but it is not the typical import structure used for the project

seems like it runs organize imports. I'll revert it. thanks

DanielRyanSmith · 2024-09-26T18:42:57Z

FYI, I'm looking into the mypy issues here regarding the generated files not being ignored, and will give an update here when I've found anything

Edit: It looks like markxiong0122 might have a solution in the comments below. 🙂

markxiong0122 · 2024-09-28T05:37:56Z

Let's take a look at the errors for the non generated files:

api/reviews_api.py:60: error: Item "None" of "GetVotesResponse | None" has no attribute "to_dict"  [union-attr]
api/reviews_api.py:149: error: Item "None" of "GetGateResponse | None" has no attribute "to_dict"  [union-attr]
api/reviews_api.py:158: error: Item "None" of "PostGateRequest | None" has no attribute "assignees"  [union-attr]
api/reviews_api.py:166: error: Argument 5 to "notify_assignees" has incompatible type "list[str] | Any | None"; expected "list[str]"  [arg-type]
api/component_users.py:55: error: Item "None" of "ComponentUsersRequest | None" has no attribute "owner"  [union-attr]
api/component_users.py:64: error: Item "None" of "ComponentUsersRequest | None" has no attribute "owner"  [union-attr]
api/comments_api.py:94: error: Item "None" of "CommentsRequest | None" has no attribute "comment"  [union-attr]
api/comments_api.py:95: error: Item "None" of "CommentsRequest | None" has no attribute "post_to_thread_type"  [union-attr]
api/comments_api.py:114: error: Argument 4 to "notify_subscribers_of_new_comments" has incompatible type "str | Any | None"; expected "str"  [arg-type]
api/comments_api.py:124: error: Argument 5 to "post_comment_to_mailing_list" has incompatible type "str | Any | None"; expected "str"  [arg-type]
api/comments_api.py:131: error: Item "None" of "PatchCommentRequest | None" has no attribute "comment_id"  [union-attr]
api/comments_api.py:138: error: Item "None" of "PatchCommentRequest | None" has no attribute "is_undelete"  [union-attr]

There are two types of errors union-attr and arg-type

`union-attr` example.

In reviews_api.py, it complains about this line:

    return GetVotesResponse.from_dict({'votes': dicts}).to_dict()

This is because from_dict with this new openapi config returns Optional[Self]. So the linter is warning that we need to be careful about using the returned value.

    response = GetVotesResponse.from_dict({'votes': dicts})
    if not response:
      logging.error(f"unable to convert response from get_votes to GetVotesResponse dicts={dicts}")
      return self.abort(500)
    return response.to_dict()

This fixes that error.

You will need to do that for others.

arg-type

message: error: Argument 5 to "notify_assignees" has incompatible type "list[str] | Any | None"; expected "list[str]" [arg-type]

original code:

  def do_post(self, **kwargs) -> dict[str, str]:
    """Set the assignees for a gate."""
    user, fe, gate, feature_id, gate_id = get_user_feature_and_gate(
        self, kwargs)
    request = PostGateRequest.from_dict(self.request.json)
    assignees = request.assignees

    self.require_permissions(user, fe, gate)
    self.validate_assignees(assignees, fe, gate)
    old_assignees = gate.assignee_emails
    gate.assignee_emails = assignees
    gate.put()
    notifier_helpers.notify_assignees(
        fe, gate, user.email(), old_assignees, assignees)

    # Callers don't use the JSON response for this API call.
    return SuccessMessage(message='Done').to_dict()

This code actually has a union-attr that needs to be fixed first. Because it thinks that things could be optional, you need to verify first. Then this error goes away automatically

  def do_post(self, **kwargs) -> dict[str, str]:
    """Set the assignees for a gate."""
    user, fe, gate, feature_id, gate_id = get_user_feature_and_gate(
        self, kwargs)
+    request = PostGateRequest.from_dict(self.request.json)
+    if not request:
+      self.abort(400)
    assignees = request.assignees

    self.require_permissions(user, fe, gate)
    self.validate_assignees(assignees, fe, gate)
    old_assignees = gate.assignee_emails
    gate.assignee_emails = assignees
    gate.put()
    notifier_helpers.notify_assignees(
        fe, gate, user.email(), old_assignees, assignees)

    # Callers don't use the JSON response for this API call.
    return SuccessMessage(message='Done').to_dict()

#Other notes:

You will see that instances where I do from_dict on the request objects, I do self.abort(400). But in instances where I do from_dict while returning something from the database, I do self.abort(500)

Thanks for the detailed explanation! I've updated my code and now it should pass everything other than mypy errors from the generated files

markxiong0122 · 2024-09-28T05:56:10Z

Regarding the mypy errors in generated files, I found this PR of openapi-generator which already solved the bug: OpenAPITools/openapi-generator#19223. Should we update our tool to the latest release? @jcscottiii

jcscottiii · 2024-09-30T09:31:19Z

I think that's a good idea!

openapi backend only python

b3868fc

jcscottiii reviewed Sep 19, 2024

View reviewed changes

markxiong0122 added 4 commits September 20, 2024 10:37

test from_dict

ee54063

fix all errors

f7ef3fe

adjust nullability

573e5d0

empty user case

4d5c26f

markxiong0122 marked this pull request as ready for review September 22, 2024 09:52

markxiong0122 requested review from jcscottiii and DanielRyanSmith September 22, 2024 09:52

markxiong0122 added 2 commits September 22, 2024 18:51

change id to int

0081cc2

mypy path

5fff8df

DanielRyanSmith reviewed Sep 26, 2024

View reviewed changes

markxiong0122 added 2 commits September 28, 2024 13:39

address mypy errors

ee6eed7

remove extra abort

6821c2e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change OpenAPI backend generator config #4376

Change OpenAPI backend generator config #4376

markxiong0122 commented Sep 19, 2024 •

edited

Loading

jcscottiii Sep 19, 2024 •

edited

Loading

markxiong0122 Sep 20, 2024

jcscottiii Sep 20, 2024

markxiong0122 Sep 20, 2024

jcscottiii Sep 20, 2024

jcscottiii Sep 20, 2024

markxiong0122 Sep 21, 2024

markxiong0122 commented Sep 22, 2024

jcscottiii commented Sep 26, 2024 •

edited

Loading

DanielRyanSmith Sep 26, 2024

markxiong0122 Sep 28, 2024

DanielRyanSmith commented Sep 26, 2024 •

edited

Loading

markxiong0122 commented Sep 28, 2024

`union-attr` example.

arg-type

markxiong0122 commented Sep 28, 2024 •

edited

Loading

jcscottiii commented Sep 30, 2024

	return GetVotesResponse.model_construct(votes=dicts).to_dict()
	return GetVotesResponse.from_dict({'votes': dicts}).to_dict()

		from framework import csp, permissions, secrets, users, utils, xsrf
		from internals import approval_defs, notifier_helpers, user_models

Change OpenAPI backend generator config #4376

Are you sure you want to change the base?

Change OpenAPI backend generator config #4376

Conversation

markxiong0122 commented Sep 19, 2024 • edited Loading

jcscottiii Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markxiong0122 commented Sep 22, 2024

jcscottiii commented Sep 26, 2024 • edited Loading

union-attr example.

arg-type

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRyanSmith commented Sep 26, 2024 • edited Loading

markxiong0122 commented Sep 28, 2024

union-attr example.

arg-type

markxiong0122 commented Sep 28, 2024 • edited Loading

jcscottiii commented Sep 30, 2024

markxiong0122 commented Sep 19, 2024 •

edited

Loading

jcscottiii Sep 19, 2024 •

edited

Loading

jcscottiii commented Sep 26, 2024 •

edited

Loading

`union-attr` example.

DanielRyanSmith commented Sep 26, 2024 •

edited

Loading

`union-attr` example.

markxiong0122 commented Sep 28, 2024 •

edited

Loading