ModelError resulting in 500 response

[sondrelg]

Hi!

I've been trying to set up a demo app to get familiar with ormar - everything has been going very good so far, so thanks a lot for creating this 👏

I just ran into something confusing that I'm not sure is a bug or intended behaviour:

Issue

when sending in an empty field name ("": None} in a request body, ormar from here raises a ModelError which results in a 500 being returned from my endpoint.

Context

This happens in my update endpoint (but I think it would happen anywhere), which looks like this:

@author_router.put('/{item_id}/', status_code=200, response_model=Author, responses=responses['not_found'])
async def update(item_id: int, updated_author: Author) -> Author:
    try:
        author = await Author.objects.get(id=item_id)
    except NoMatch:
        raise HTTPException(status_code=404, detail="Item not found")

    await author.update(**updated_author.dict(exclude_unset=True))
    return author

The payload that triggers the error looks like this

{"name": "", "short_name": "", "": None}

And my model looks like this:

class Author(ormar.Model):
    class Meta(BaseMeta):
        tablename = 'authors'

    id: int = ormar.Integer(primary_key=True, autoincrement=True)
    name: str = ormar.String(max_length=100, unique=True)
    short_name: str = ormar.String(max_length=100)

Expected behavior

I would expect Ormar to ignore this empty field value since it doesn't correspond to anything in my model. Alternatively I would like the ModelError to result in a 422 being returned - anything other than a 500, since this would mean I have to add logic for this in every endpoint.

If I substitute the Ormar model with a Pydantic model, Pydantic seems to just ignore the field. This works:

AuthorUpdateModel = Author.get_pydantic(exclude={'id', 'books'})

@author_router.put('/{item_id}/', status_code=200, response_model=Author, responses=responses['not_found'])
async def update(item_id: int, updated_author: AuthorUpdateModel) -> Author:
    try:
        author = await Author.objects.get(id=item_id)
    except NoMatch:
        raise HTTPException(status_code=404, detail="Item not found")

    await author.update(**updated_author.dict(exclude_unset=True))
    return author

---

Is this a bug, or am I missing something important in terms of how to structure my APIs? 🙂

[collerek]

Yes, ormar does not allow you to set extra, not defined fields on a model.
In pydanitc you can control this with extra option, by default it's set to ignore https://pydantic-docs.helpmanual.io/usage/model_config/.

But you cannot change this in ormar as of now, that can be a possible enhancement.

It was not supported from the beginning as there is no way to persist this randomly added column in a database anyway. And since ormar heavily operates on its own set of fields (among others to include proper relation handling etc.), it would have to reach to pydantic to check if there are any additional fields which I wanted to avoid.

But that refers only to accepting the newly created fields, a setting for forbid (as now) and ignore (a new option) seems handy and quite easy to implement, so can you please open an issue for this?

Why are you passing empty dict value anyway? :)

[collerek]

Ormar does not raise 500, it's fastapi.
That basically means that it was an unhandled exception.

Pydantic validation errors are hijacked by fastapi and hence the 422.

So I would have to change the exception to pydantic.ValidationError, which would be not really true, and moreover a breaking change - so I would like to avoid this.

But! :)
In fastapi you don't handle the same errors in every endpoint, simply register your custom exception handler for ormar.ModelError.

# app is a fastapi instance
@app.exception_handler(ModelError)
async def ormar_exception_handler(request: Request, exc: ModelError):
    return JSONResponse(
        status_code=422,
        content={"message": "{}: {}".format(exc.__class__.__name__, "{}".format(exc)),
                 "description": "Ormar doesn't like me! :( "}
    )

And you are done.

[sondrelg]

This actually solves my issue - I actually prefer to forbid extra parameters; I think it makes more sense than ignoring them, so I won't open an issue unless you want me to 🙂

Thanks again!

[collerek]

If that works for you, for now, I'm good with forbidding for now too :D

[collerek]

Let me know what you think of ormar, I'm collecting feedback! (Critique also welcomed!) 😊

[sondrelg]

So far we're liking it a lot, but we're in the process of implementing it - we have no experience running it in production yet, so it might be a little early to give good feedback🙂 Our initial hope was that ormar would help eliminate the need for duplicate database/pydantic model definitions - which it certainly helps with; and the Django-like syntax has also been great, as that's something we're already using in the company.

So tl;dr: a little early to know, but only positives so far 🎉 and thanks for the extremely quick response to my issue 👏