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

Incorrect input field specification for POST methods #26

Closed
chrisjsewell opened this issue Jun 15, 2021 · 4 comments · Fixed by #29
Closed

Incorrect input field specification for POST methods #26

chrisjsewell opened this issue Jun 15, 2021 · 4 comments · Fixed by #29
Labels
bug Something isn't working

Comments

@chrisjsewell
Copy link
Member

chrisjsewell commented Jun 15, 2021

Basically the fields you can return from an ORM entity (via QueryBuilder) are not the same as the fields available to create an ORM entity, e.g. here it suggests you can specify the id and user_id, which is not true:

image

equally the way the pydantic models are set up in models.py, IMO, feel a bit "off":

class Group(AiidaModel):
    """AiiDA Group model."""

    _orm_entity = orm.Group

    id: Optional[int] = Field(description="Id of the object")
    label: str = Field(description="Used to access the group. Must be unique.")
    type_string: Optional[str] = Field(description="Type of the group")
    user_id: Optional[str] = Field(description="Id of the user that created the node.")
    description: Optional[str] = Field(description="Short description of the group.")

Here, everything except for label is set as Optional, since this is the only required field to create a group. But this does not accurately describe what is stored in the database, and thus what you would return from a GET method, where things like id are not optional (i.e. it should never be None).

Finally, fields like user_id will be returned from e.g. QueryBuilder().append(Group, project=["**"]), because they are a field on the database table, but they will always be None when using from_orm, because they are not exposed as an attribute of the AiiDA ORM class.

FYI this is what I was talking about in #17 (comment) @ltalirz

Also note the /groups POST method is currently called create_user 😜

@chrisjsewell chrisjsewell added the bug Something isn't working label Jun 15, 2021
@chrisjsewell
Copy link
Member Author

cc @NinadBhat

basically we should consider either having separate models for getting/creating, or encoding additional information on each Field, regarding which fields are available/required for creation

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Jun 15, 2021

and yeh this is why I have currently kept https://github.com/aiidateam/aiida-restapi/blob/master/aiida_restapi/aiida_db_mappings.py separate from models.py, since aiida_db_mappings defines accurately (I hope) the returned fields from the QueryBuilder, which are not exactly compatible with the models currently defined in models.py (more geared towards POST requests)

@ltalirz
Copy link
Member

ltalirz commented Jun 15, 2021

in production code, it seems that people do indeed define separate models for

  • POST response (create)
  • PUT response (update)
  • GET response
  • Full information in DB (e.g. hashed passwords)

In our case, for most entities (except User) we can probably get away with one model for GET and one for POST.
What do you think?

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Jun 15, 2021

we can probably get away with one model for GET and one for POST. What do you think?

yeh indeed we should definitely at least split those two up. In terms of e.g. user_id, I think they can be useful for the client, but maybe it goes against the aiida "ORM concept", to be accessing things that are not specifically exposed by the ORM class.

TBH its made it a bit "confusing", that you have the AiiDA ORM classes, which are not technically the same as the backend sqlalchemy/django ORM classes, but that with the QueryBuilder you specify / get back the fields of the "backend" ORM classes?
(the classic being this annoying translation of id -> pk, and also name -> label on computers)

(this also relates to #1, i.e. where can you get what the "source of truth" is for the ORM fields 🤷)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants