Conversation
cvat/apps/engine/signals.py
Outdated
|
|
||
| @receiver(post_save, sender=User, dispatch_uid=__name__ + ".save_user_handler") | ||
| def __save_user_handler(instance: User, **kwargs): | ||
| if kwargs.get("created") and kwargs.get("raw"): |
There was a problem hiding this comment.
The created check doesn't seem necessary. If we skip the logic on raw creates, then why wouldn't we do that on raw updates?
There was a problem hiding this comment.
The problem I faced was related to extra object creation, so I focused the fix only on this. I'm not sure why this may be called without object creation, but if you know such cases, I'm open to covering this as well.
There was a problem hiding this comment.
Actually, in pre_save we don't have created, so it makes sense to align the logic.
There was a problem hiding this comment.
Nope, doesn't seem to work for any raw - 4a84adb has a number of failed tests.
There was a problem hiding this comment.
Okay, curious...
I'm fine with keeping it as-is for most cases, but for webhooks we do need equivalent checks in the pre_save and post_save handlers; post_save depends on pre_save having run first.
There was a problem hiding this comment.
Ok, updated. pre_save only adds extra fields to the passed instance to be used later in post_save, so removed all changes from pre_save.
There was a problem hiding this comment.
Where do these changes come from?
There was a problem hiding this comment.
I'm not sure about the source of these changes. I can reproduce it on the develop branch as well.
There was a problem hiding this comment.
All this seems to do is put the annotations in a different order. Since the order appears to be non-deterministic, I don't see much reason to commit this. I think we should update the dump script to sort the output before saving.
There was a problem hiding this comment.
Yeah, let's revert this
zhiltsov-max
force-pushed
the
zm/prevent-signals-in-test-db-setup
branch
from
3702a44 to
265dfba
Compare
zhiltsov-max
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #9272 +/- ##
===========================================
- Coverage 74.18% 74.16% -0.03%
===========================================
Files 429 428 -1
Lines 44970 44964 -6
Branches 3918 3918
===========================================
- Hits 33363 33348 -15
- Misses 11607 11616 +9
🚀 New features to boost your workflow:
|
|
zhiltsov-max
changed the title
<!-- Raise an issue to propose your change (https://github.com/cvat-ai/cvat/issues). It helps to avoid duplication of efforts from multiple independent contributors. Discuss your ideas with maintainers to be sure that changes will be approved and merged. Read the [Contribution guide](https://docs.cvat.ai/docs/contributing/). --> <!-- Provide a general summary of your changes in the Title above --> ### Motivation and context Resolves #9382 I have ran into the same issue, I have done some searching and saw that the LDAP `create_user` was changed in #9272 for the LDAP implementation to have two new arguments `created` and `raw` which is not needed in the [Django LDAP Authentication](https://github.com/django-auth-ldap/django-auth-ldap/blob/master/django_auth_ldap/backend.py), I think the change was just meant to fall in line with the [django post_save signal](https://docs.djangoproject.com/en/5.1/ref/signals/#django.db.models.signals.post_save) ### How has this been tested? <!-- Please describe in detail how you tested your changes. Include details of your testing environment, and the tests you ran to see how your change affects other areas of the code, etc. --> I removed the lines and rebuilt and tried logging in with LDAP and it succeed. ### Checklist <!-- Go over all the following points, and put an `x` in all the boxes that apply. If an item isn't applicable for some reason, then ~~explicitly strikethrough~~ the whole line. If you don't do that, GitHub will show incorrect progress for the pull request. If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [x] I submit my changes into the `develop` branch - [x] I have created a changelog fragment <!-- see top comment in CHANGELOG.md --> - [ ] ~~I have updated the documentation accordingly~~ - [ ] ~~I have added tests to cover my changes~~ - [x] I have linked related issues (see [GitHub docs]( https://help.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword)) ### License - [x] I submit _my code changes_ under the same [MIT License]( https://github.com/cvat-ai/cvat/blob/develop/LICENSE) that covers the project. Feel free to contact the maintainers if that's a concern. --------- Co-authored-by: Kira Pearce <kira.pearce@digital-workbench.de> Co-authored-by: Andrey Zhavoronkov <andrey@cvat.ai>
3 participants
Motivation and context
To setup test DB CVAT runs
python manage.py loaddata .... This command populates the DB with objects fromtests/python/shared/assets/cvat_db/data.json. This action also invokes signals on the server normally, as specified for models. In some cases it can lead to unexpected errors with duplicate keys in DB - because, for example, a parent object is loaded first, a signal runs to create a nested object, and then the nested object is created once again byloaddatawhen time comes. I'm surprised the current test startup is working without extra steps to disable signals.References:
rawparameter inpost_datahttps://docs.djangoproject.com/en/5.1/ref/signals/#post-saveAnother problem is that sometimes it's possible to get test suite startup check failed if the server check is invoked before the server is started.
Changes:
loaddataHow has this been tested?
Checklist
developbranchLicense
Feel free to contact the maintainers if that's a concern.