-
Notifications
You must be signed in to change notification settings - Fork 7
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
IA-3456 improve get or create for orgunit (duplicate on uuid) #1661
base: main
Are you sure you want to change the base?
Conversation
@@ -686,6 +686,7 @@ def instance_logs(self, request, pk=None, logId=None): | |||
|
|||
def import_data(instances, user, app_id): | |||
project = Project.objects.get_for_user_and_app_id(user, app_id) | |||
default_version = project.account.default_version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's possible to not have a default version on an account. I think that situation would probably blow up anyways, but not sure if we should consider it here 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beygorghor I think we already had to code special cases when there is no default_version so it can happen right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes indeed, the frontend allows no default version
def import_data(org_units, user, app_id): | ||
new_org_units = [] | ||
project = Project.objects.get_for_user_and_app_id(user, app_id) | ||
org_units = sorted(org_units, key=get_timestamp) | ||
|
||
project = Project.objects.get_for_user_and_app_id(user, app_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate of line 347?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments from Bram, otherwise it seems ok to me
@@ -686,6 +686,7 @@ def instance_logs(self, request, pk=None, logId=None): | |||
|
|||
def import_data(instances, user, app_id): | |||
project = Project.objects.get_for_user_and_app_id(user, app_id) | |||
default_version = project.account.default_version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beygorghor I think we already had to code special cases when there is no default_version so it can happen right ?
@mestachs In order to deblock this PR, do you think it would be a good idea to add a conditional unique index when the |
80f5275
to
e9c3272
Compare
Conditional indexes to add manually on big servers: CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS iaso_orgunit_uuid_version_idx_unicity_newer ON iaso_orgunit (uuid, version_id)
WHERE
uuid IS NOT NULL AND version_id IS NOT NULL AND created_at >= '2025-01-14';
CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS iaso_entity_uuidx_unicity_newer ON iaso_entity (uuid)
WHERE
uuid IS NOT NULL AND created_at >= '2025-01-14';
CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS iaso_instance_uuidx_unicity_newer ON iaso_instance (uuid)
WHERE
uuid IS NOT NULL AND created_at >= '2025-01-14'; |
@mathvdh @madewulf @bmonjoie Do you see any issues with manually adding the indexes in my above comment? Since UUIDs for instances and entities are generated on the mobile app I don't think they should be unique only per project or something like that. Making them real UUIDs feels like the best option to me. For org units we're enforcing uniqueness only per version since @mestachs discovered that import/export/reimport geopackage is putting the same UUID in 2 source versions. |
@bramj I don't see any problem with it (except for duplicate line 350 as commented above). Is the plan to cleanup duplicate uuids and apply the index unconditionally after that ? Or what's the plan ? |
Ideally, yes that would be the plan, but to be honest not sure how doable that is. |
To minize duplicate and be "ready" when we will introduce the unique index
Related JIRA tickets : IA-3456
Self proofreading checklist
Doc
Changes
see #1652
How to test
see #1652
but the mobile unit test does that
for a more "real life" reproduction : https://gist.github.com/mestachs/2cc50bf310996d14cc48623a05d42caf
note that we have historical data so we will need an intermediate cleanup
create the a temporary index as in the _notes.md
then really cleanup "old data" and all environments (harder)
Print screen / video
Upload here print screens or videos showing the changes
Notes
Things that the reviewers should know: known bugs that are out of the scope of the PR, other trade-offs that were made.
If the PR depends on a PR in bluesquare-components, or merges into another PR (i.o. main), it should also be mentioned here