Tell me what you DON'T like about ormar? [Feedback Wanted]

[collerek]

Hi,

I would like to get some feedback about what users don't like about the package.
You can always put a word or two about what you like but the main focus is to find areas of improvement.

Cheers!

[bilyalov]

Uncomfortable to work with FKey

Example:

class Department(ormar.Model):
    class Meta:
        database = database
        metadata = metadata

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


class Course(ormar.Model):
    class Meta:
        database = database
        metadata = metadata

    id: int = ormar.Integer(primary_key=True)
    name: str = ormar.String(max_length=100)
    completed: bool = ormar.Boolean(default=False)
    department: Optional[Department] = ormar.ForeignKey(Department)

When I create a Course that references Department, I need to add an object to the department field (something like {'id': 1}), but not a numeric value
course = await Course.objects.create(department={'id': 1})
it would be better like this
course = await Course.objects.create(department=1)
or
course = await Course.objects.create(department_id=1)

[collerek]

You should be able to pass the underlying value, so course = await Course.objects.create(department=1) should work.
What happens in your case?

[bilyalov]

You should be able to pass the underlying value, so course = await Course.objects.create(department=1) should work.
What happens in your case?

I see this error:
AttributeError: 'int' object has no attribute 'get'

post_info = {
    "title": "My post",
    "link": 1
}

self.post = await Post.objects.create(**post_info)

# Models
class Post(ormar.Model):
    id: int = ormar.Integer(primary_key=True)
    title: str = ormar.String(max_length=500)
    link: PageLink = ormar.ForeignKey(PageLink, related_name='posts', ondelete="CASCADE")

    class Meta(BaseMeta):
        tablename = 'posts'

class PageLink(ormar.Model):
    id: int = ormar.Integer(primary_key=True)
    value: str = ormar.String(max_length=2048)
    country: str = ormar.String(max_length=1000)

    def __str__(self):
        return f'{self.id}'

    class Meta(BaseMeta):
        tablename = 'pagelinks'

Stack:

  File ".../venv/lib/python3.9/site-packages/ormar/queryset/queryset.py", line 1018, in create
    new_kwargs = self.model.prepare_model_to_save(new_kwargs)
  File ".../venv/lib/python3.9/site-packages/ormar/models/mixins/save_mixin.py", line 40, in prepare_model_to_save
    new_kwargs = cls.substitute_models_with_pks(new_kwargs)
  File ".../venv/lib/python3.9/site-packages/ormar/models/mixins/save_mixin.py", line 130, in substitute_models_with_pks
    model_dict[field] = field_value.get(target_pkname)
AttributeError: 'int' object has no attribute 'get'

[collerek]

That's a bug - created an issue for it, thanks for catching that!

[collerek]

Can you tell me which version of ormar do you use as I cannot reproduce?

from typing import List, Optional

import databases
import pytest
import sqlalchemy

import ormar
from tests.settings import DATABASE_URL

database = databases.Database(DATABASE_URL, force_rollback=True)
metadata = sqlalchemy.MetaData()


class BaseMeta(ormar.ModelMeta):
    metadata = metadata
    database = database


class PageLink(ormar.Model):
    class Meta(BaseMeta):
        tablename = 'pagelinks'

    id: int = ormar.Integer(primary_key=True)
    value: str = ormar.String(max_length=2048)
    country: str = ormar.String(max_length=1000)


class Post(ormar.Model):
    class Meta(BaseMeta):
        tablename = 'posts'

    id: int = ormar.Integer(primary_key=True)
    title: str = ormar.String(max_length=500)
    link: PageLink = ormar.ForeignKey(PageLink, related_name='posts',
                                      ondelete="CASCADE")


class Department(ormar.Model):
    class Meta:
        database = database
        metadata = metadata

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


class Course(ormar.Model):
    class Meta:
        database = database
        metadata = metadata

    id: int = ormar.Integer(primary_key=True)
    name: str = ormar.String(max_length=100)
    completed: bool = ormar.Boolean(default=False)
    department: Optional[Department] = ormar.ForeignKey(Department)


@pytest.fixture(autouse=True, scope="module")
def create_test_database():
    engine = sqlalchemy.create_engine(DATABASE_URL)
    metadata.create_all(engine)
    yield
    metadata.drop_all(engine)


@pytest.mark.asyncio
async def test_create_primary_models():
    async with database:
        async with database.transaction(force_rollback=True):
            link = await PageLink(id=1, value='test', country='USA').save()
            await Post.objects.create(title="My post", link=link.id)
            post_check = await Post.objects.select_related('link').get()
            assert post_check.link == link


@pytest.mark.asyncio
async def test_create_primary_models():
    async with database:
        async with database.transaction(force_rollback=True):
            dept = await Department(name='Department test').save()
            await Course(name='Test course', department=dept.id).save()

Those tests pass in latest version.

[johnthagen]

In general one of the things I've noticed is that ormar can be a little inefficient. If you compare .values() to using the models there is a fair amount of overhead.

I think part of this is that it seems like every instance gets validated twice. As in the validation seems to run when you pull things out of the database, and then it's run again when the model is sent out of FastAPI.

Projects like Django and DRF I think assume that stuff coming out of the database doesn't need to validated (e.g. if I have an Integer field, there is no need to call int() on it when making the Model instance).

I'm not sure how practical doing any of this would be for ormar since it relies on Pydantic and digging into the internals could be an issue, but it's just something I've noticed.

So maybe some kind of "inbound only" validation could be helpful to make ormar more efficient?

[collerek]

Thanks for the feedback!

Yes, I noticed the same, full validation slows things down and since the data comes from db it should be trusted by default.

Every gain in performance is more than welcome, especially that fastapi parses the data several times on it's own (there are issues about this in repo issues).

I plan to implement construct() as per #318 and switch models creation to this one instead of using initialization for data read from db, will see how much that speeds things up.

[collerek]

I added construct in 0.10.21 but before switching I need to run some profiling.

The reason is that values simply dump your rows of data into dictionaries, while other queries have to extract nested models into child model dictionaries and combine duplicated parts of rows into one model with multiple child models and that might take more time than validation itself (none of this is done in values).

So I don't want to blindly pre-optimize especially that simply switching to construct raised quite a few errors for each change I tried, so that won't be as simple as changing init to construct, therefore I need to verify where are the bottlenecks first.

[johnthagen]

@collerek I suspect it also has to do with the Model structure and types. For example, I could imagine str() calls potentially being faster than float() parsing. I also found that datetime types seemed to be more expensive.

So based on the types of the Pydantic model (and number of model instances, e.g. thousands/hundreds of thousands) I'm sure that will all effect performance results.

[igormorgado]

I don't like much (of course I can live with that) to Model.objects.create(). I do prefer to use Model.create() (and all related methods).

[LonelyVikingMichael]

Hi Collerek

One thing that stands out to me is the add() method on a ManyToMany field instance only accepts a Model argument, whereas a ForeignKey accepts either a Model instance or just the primary key value. This means that we need to query and build each related model before we can add() them.

Consider an API route where we POST new Students, who can have multiple Courses. Given a JSON payload:

{
    "name": "Bark",
    "courses": [
        {"id": "11ea82fe-72c3-4029-a683-7572d934c320"},
        {"id": "5db60d96-5577-4ae7-bfa8-ff4941b10871"},
    ],
   ...
}

And a route:

StudentRequest = Student.get_pydantic(include={'name': ..., 'courses': 'id'})
@app.post('/students', response_model=Student.get_pydantic(include={'id', 'some_other_attr'})
async def create_student(student: StudentRequest):
    student_db = await Student.objects.create(**student.dict())
    for course in student.courses:
        course_db = await Course.objects.get(pk=course.id)
        await student_db.add(course_db)
    return student_db

It would be much more convenient to be able to call await student_db.add(course.id) without the extra query. Perhaps I am missing something?

Other than that, using Ormar has been a pleasure. I especially enjoy being able to dynamically add filters to a list such as
filters = [Student.name.icontains('xyz'), ...] and then calling Student.objects.filter(*filters).all()

Thanks for your work.

[Strernd]

I have the same issue. This is not intuitive when using this with a Rest API.

[Strernd]

First I want to thank you, @collerek for creating this package. I really like the Fastapi integration.

We decided to use Ormar for a new internal service and are bumping into problems as soon as we're trying to do non-trivial things.
They problems seem to be related to many-to-many relationships and I feel there should be more examples on how to use more complicated models (with relations) with Fastapi.

This are the two specific problems we encountered.

• Response model validation fails on nested model. #616
• What is the ormar way to create an entity with relations via Fastapi? #617

[vvanglro]

Maybe should only concentrate on the orm project, don't integrate pydantic, many times, it will be clearer to separate the orm model from the pydantic model.

[SepehrBazyar]

Hi @collerek
First, I want to thank you for this wonderful package.

I think this related to encode/databases:
It would be much more efficient if after executing the query we saved the .rowcount value returned from SQLAlchemy Core, and for example, when getting its size with len() in __len__, instead of calculating the length of the list, we used this number.