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

[16.0][IMP] extendable_fastapi: Add generics schemas #380

Merged
merged 2 commits into from
Oct 13, 2023

Conversation

lmignon
Copy link
Contributor

@lmignon lmignon commented Oct 11, 2023

Add base schemas to be used to define pagination and paginated results when defining a search router.

@lmignon lmignon force-pushed the 16.0-extendable-fastapi-paging-models branch 3 times, most recently from 4c0428a to dfdc908 Compare October 11, 2023 07:58
@@ -24,3 +26,28 @@ class StrictExtendableBaseModel(
* validate_assignment=True: revalidate the model when the data is changed (default is False)
* extra="forbid": Forbid any extra attributes (default is "ignore")
"""


Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sbidoul @sebastienbeau @qgroulard I'm not sure it's the right place to define these base schemas. Even if they are mainly used with fastapi, we also use the StrictExtendableBaseModel when we define models to be stored into a search engine. This leads to a useless dependency on fastapi. Do we've to create a specific addon for the StrictExtendableBaseModel? I could also move it to the extendable_fastapi python lib.

Copy link
Member

Choose a reason for hiding this comment

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

I could also move it to the extendable_fastapi python lib.

But this PR is for extendable_fastapi ?

Question: why would one want to extend PagedCollection or Paging? Or is this necessary for compatibility when T is extendable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, at least for PagedCollection, it's necessary for compatibility when T is extendable.

Regarding StrictExtendableBaseModel, it's not defined at the right place or we should not use-it when defining schemas for the search engine index.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes I see. So you mean defining StrictExtendableBaseModel in extendable_pydantic, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Member

Choose a reason for hiding this comment

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

Ok, makes sense to me. Let's do that.

# extendable models. It's based on the StrictExtendableBaseModel to ensure
# a strict validation when used within the odoo fastapi framework.

total: int
Copy link
Member

Choose a reason for hiding this comment

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

Probably too late but... is count a better field name than total?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, It's not too late. What do you propose? count ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes count, but it's not very important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wording is important. I find a way to ensure the consistency with the same version of the model defined in the fastapi odoo addon but not supporting extendable items. The idea is to deprecated the use of the totalfield but keeping the compatibility on both side: json and python

import json
from typing import Annotated
from pydantic import BaseModel, computed_field, Field, AliasChoices


class Paginated(BaseModel):
    count: Annotated[
        int,
        Field(
            ...,
            desciption="Count of items into the system.\n Replaces the total field which is deprecated",
            validation_alias=AliasChoices("count", "total"),
        ),
    ]

    @computed_field()
    @property
    def total(self) -> int:
        return self.count

    @total.setter
    def total(self, value: int):
        self.count = value


p = Paginated(count=10)
print(p.model_dump_json())
>> {"count":10,"total":10}

p = Paginated(total=20)
print(p.model_dump_json())
>> {"count":20,"total":20}

p.total = 30
print(p.model_dump_json())
>> {"count":30,"total":30}

print(json.dumps(p.model_json_schema(), indent=2))
>> {
  "properties": {
    "count": {
      "desciption": "Count of items into the system.\n Replaces the total field which is deprecated",
      "title": "Count",
      "type": "integer"
    }
  },
  "required": [
    "count"
  ],
  "title": "Paginated",
  "type": "object"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've modified fastapi to ensure consistency between the two declarations in this template

A specialized verion of the PagedCollection from
odoo.addons.fastapi.schemas but supporting extendable models as type a
args.
@lmignon lmignon force-pushed the 16.0-extendable-fastapi-paging-models branch from dad0fb4 to 4b196ef Compare October 12, 2023 13:08
@lmignon
Copy link
Contributor Author

lmignon commented Oct 12, 2023

@qgroulard @AnizR What do you think about this PR?

Copy link
Contributor

@AnizR AnizR left a comment

Choose a reason for hiding this comment

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

Great!

Copy link
Contributor

@qgroulard qgroulard left a comment

Choose a reason for hiding this comment

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

Looks good.

Copy link
Member

@sebastienbeau sebastienbeau left a comment

Choose a reason for hiding this comment

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

LTGM, tested

@sebastienbeau
Copy link
Member

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 16.0-ocabot-merge-pr-380-by-sebastienbeau-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 65cb804 into OCA:16.0 Oct 13, 2023
4 of 6 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at e9b65cf. Thanks a lot for contributing to OCA. ❤️

@lmignon lmignon deleted the 16.0-extendable-fastapi-paging-models branch October 13, 2023 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants