aiidateam / aiida-restapi

AiiDA Web API for data queries and workflow management.
https://aiida-restapi.readthedocs.io
MIT License
10 stars 7 forks source link

Incorrect input field specification for POST methods #26

Closed chrisjsewell closed 3 years ago

chrisjsewell commented 3 years ago

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 https://github.com/aiidateam/aiida-restapi/pull/17#discussion_r651289244 @ltalirz

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

chrisjsewell commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

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 commented 3 years ago

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 🤷)