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

Column 'id' cannot be null issue during observers #777

Closed
lucadelmonte opened this issue Jul 28, 2022 · 18 comments · Fixed by #779
Closed

Column 'id' cannot be null issue during observers #777

lucadelmonte opened this issue Jul 28, 2022 · 18 comments · Fixed by #779
Labels
bug An existing feature is not working as intended

Comments

@lucadelmonte
Copy link

lucadelmonte commented Jul 28, 2022

When using eager loading on the documentation it's stated that the number of queries on DB will be just one, but using eager loading when having a relationship not hidden (i.e. serializable ) the number of queries with eager loading is greater than without eager loading.

Example To Reproduce
I created a small repo to illustrate the problem
https://github.com/lucadelmonte/masonite_eager_bug
you can run the run.sh file, it will create the tables on a sqllite db.

Expected behavior
Eager loading should only execute 2 queries, one for the Parent model and one for the childs (one for relationship)

Desktop (please complete the following information):

  • OS:Linux
  • Version: Ubuntu 21.10

What database are you using?
tested on both mysql, mariadb and SqlLite
Masonite ORM [2.15.0]

Possible solution
The additional queries are executed by

relation.pluck(self.local_key, keep_nulls=False).unique(),
the call to pluck on BaseRelationship which in turn calls serialize on all the child objects before executing the "eager loading query" select * from X IN (....), i managed to fix the problem replacing the call to pluck with a one to Collection(relation._get_value(self.local_key)).unique()
def _get_value(self, key):

Additional
Can you please apply the patch to the version 2.15.0, because we tried to upgrade to 2.18.2 and it has some incompatibilities.

@lucadelmonte lucadelmonte added the bug An existing feature is not working as intended label Jul 28, 2022
@lucadelmonte
Copy link
Author

Hello @josephmancuso , thank you very much for fixing it!

Woul dit be possible to add the fix to version 2.15.0?
Thank you.

@josephmancuso
Copy link
Member

@lucadelmonte no. What issues are you running into with 2.18.3? 2.18.3 should have no backwards incompatible changes to 2.15

@federicoparroni
Copy link
Contributor

Hello! After the upgrade from 2.15.0 to 2.18.1, we are experiencing several issues, the first one I can mention is the following:

from masoniteorm.models import Model
from masoniteorm.scopes import scope
from masoniteorm.relationships import has_many, has_one

class Backup(Model):
    __table__ = 'backups'
    __guarded__ = []

    @classmethod
    def _boot(cls):
        cls.add_global_scope(
            lambda query: query.order_by('created_at', 'desc'))
        super(Backup, cls)._boot()

    @has_many('id', 'backup_id')
    def restores(self):
        return Restore

    @has_one('fallback_backup_id', 'id')
    def restore(self):
        return Restore


class Restore(Model):
    __appends__ = [
        'fallback_backup_id',
        'backup_id'
    ]

    @belongs_to('backup_id', 'id')
    def backup(self):
        return Backup

    @has_one('id', 'fallback_backup_id')
    def fallback(self):
        return Backup

    def createFallback(self):
        backup = Backup.create({
            'website_id': self.backup.website_id,
            'name': 'Automatic backup before restore',
        })
        self.attach('fallback', backup)
        self.save()

class RestoreObserver(object):
    def creating(self, restore):
        restore.backup.status = 'restoring'
        restore.backup.save()

    def created(self, restore):
        restore.createFallback()  # EXCEPTION!!!

Restore.observe(RestoreObserver())


if __name__ == "__main__":
    backup = Backup.create(dict(
         website_id=website.id,
         name=name if name else f"Backup {datetime.now().isoformat()}",
         provider=provider,
         options=options,
         status=status if status else 'creating',
    ))
    restore = Restore.create(dict(
        backup_id=backup.id,
        fallback_backup_id=None,
    ))

The restore creation in the observer created function fails with:

masoniteorm.exceptions.QueryException: (1048, "Column 'id' cannot be null")

@josephmancuso
Copy link
Member

Upgrade to latest (2.18.4) and let me know if you get same issue

@josephmancuso
Copy link
Member

josephmancuso commented Aug 1, 2022

also what is the primary key on the Backup model?

@federicoparroni
Copy link
Contributor

Thank you, I am going to try with the latest, 2.18.4 asap.
This is the backup table migration:

from masoniteorm.migrations import Migration

class CreateBackupsTable(Migration):

    def up(self):
        """
        Run the migrations.
        """
        with self.schema.create('backups') as table:
            table.big_increments('id')
            table.string('name', 191)
            table.string('provider', 191).nullable().default(None)
            table.text('options').nullable()
            table.enum('status', ['creating','created','deleting','restoring','restored']).default('creating')
            table.timestamps()

    def down(self):
        """
        Revert the migrations.
        """
        self.schema.drop('backups')

@josephmancuso josephmancuso changed the title Eager loading not working as intended on not hidden relationships Column 'id' cannot be null issue during observers Aug 1, 2022
@josephmancuso josephmancuso reopened this Aug 1, 2022
@josephmancuso
Copy link
Member

If its still an issue I can look into it within the next day or 2

@josephmancuso
Copy link
Member

also if you can provide a larger stack trace it will help pinpoint which line is causing the issues

@federicoparroni
Copy link
Contributor

Thank you @josephmancuso. Tomorrow I will provide the full stack trace, test with 2.18.4 and let you know if the issue is still there

@federicoparroni
Copy link
Contributor

This is the stack trace with 2.18.1:

main.py:256: in create_restore
    return Restore.create(dict(
venv/lib/python3.10/site-packages/masoniteorm/models/Model.py:537: in create
    return cls.builder.create(dictionary, id_key=cls.__primary_key__)
venv/lib/python3.10/site-packages/masoniteorm/query/QueryBuilder.py:550: in create
    self.observe_events(model, "created")
venv/lib/python3.10/site-packages/masoniteorm/observers/ObservesEvents.py:6: in observe_events
    getattr(observer, event)(model)
my-models/restore.py:41: in created
    restore.createFallback()
my-models/restore.py:31: in createFallback
    self.attach('fallback', backup)
venv/lib/python3.10/site-packages/masoniteorm/models/Model.py:1021: in attach
    return related.attach(self, related_record)
venv/lib/python3.10/site-packages/masoniteorm/relationships/BaseRelationship.py:143: in attach
    return related_record.update(
venv/lib/python3.10/site-packages/masoniteorm/models/Model.py:757: in method
    return getattr(self.get_builder(), attribute)(*args, **kwargs)
venv/lib/python3.10/site-packages/masoniteorm/query/QueryBuilder.py:1430: in update
    self.new_connection().query(self.to_qmark(), self._bindings)

@federicoparroni
Copy link
Contributor

This is the stack trace with 2.18.4 (error is happening also here):

main.py:256: in create_restore
    return Restore.create(dict(
venv/lib/python3.10/site-packages/masoniteorm/models/Model.py:557: in create
    return cls.builder.create(dictionary, id_key=cls.__primary_key__)
venv/lib/python3.10/site-packages/masoniteorm/query/QueryBuilder.py:555: in create
    self.observe_events(model, "created")
venv/lib/python3.10/site-packages/masoniteorm/observers/ObservesEvents.py:6: in observe_events
    getattr(observer, event)(model)
my-models/restore.py:41: in created
    restore.createFallback()
my-models/restore.py:31: in createFallback
    self.attach('fallback', backup)
venv/lib/python3.10/site-packages/masoniteorm/models/Model.py:1041: in attach
    return related.attach(self, related_record)
venv/lib/python3.10/site-packages/masoniteorm/relationships/BaseRelationship.py:142: in attach
    return related_record.update(
venv/lib/python3.10/site-packages/masoniteorm/models/Model.py:777: in method
    return getattr(self.get_builder(), attribute)(*args, **kwargs)
venv/lib/python3.10/site-packages/masoniteorm/query/QueryBuilder.py:1440: in update
    self.new_connection().query(self.to_qmark(), self._bindings)

@federicoparroni
Copy link
Contributor

Hello! May I help more on this problem?

@josephmancuso
Copy link
Member

@federicoparroni You gave the stack trace but what exactly is the error message? Its still masoniteorm.exceptions.QueryException: (1048, "Column 'id' cannot be null")?

@federicoparroni
Copy link
Contributor

@josephmancuso Yes, exactly, the error is still that one

@lucadelmonte
Copy link
Author

Hello @josephmancuso do you have any updated on this issue? is there any way we could help fixing it?

@federicoparroni
Copy link
Contributor

federicoparroni commented Sep 1, 2022

Hello @josephmancuso! I discovered that with the latest version 2.18.4 the observer issue does not happen if I invert the attach logic:

class Restore(Model):
    # ....
    def createFallback(self):
        backup = Backup.create({
            'website_id': self.backup.website_id,
            'name': 'Automatic backup before restore',
        })
        # self.attach('fallback', backup)  # changed this
        backup.attach('restore', self)     # to this
        self.save()

This has been changed here: v2.15.0...v2.18.4#diff-7d53b4b210acd0628e571405097338130b3d03a2e32eb6929001f959697cb7d8R142-R143
I did not know if this is the intended behaviour for the has_one relation... should it work both ways?

@josephmancuso
Copy link
Member

So you fixed this in your code right?

@federicoparroni
Copy link
Contributor

Yes, it's fixed on our side, but is this the intended behaviour?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An existing feature is not working as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants