Fix updating ID for resource #3239
Conversation
The front-end is using the client side ID generation to send in parallel all the chunks so not sure it's possible without changing the API (for exemple having a client side UUID then a real UUID saved in the database at the end of the upload process)… https://github.com/datagouv/front-end/blob/700fa2e57f9ae347c7b59e8eb1e5dbbff659a475/utils/datasets.ts#L175-L214 |
There was a problem hiding this comment.
Thank you for the investigation and proposed solution!
I don't think the resource id field is currently set by the frontend.
The uuid header in the multipart isn't currently used to create the Resource object, it is only used to identify the chunks the file belong to.
Comparing the uuid with the resource id shows they differ in this chunk upload test case.
I think we should make sure the resource id field is entirely readonly instead.
| @@ -377,8 +383,9 @@ class ResourcesAPI(API): | |||
| def post(self, dataset): | |||
| """Create a new resource for a given dataset""" | |||
| ResourceEditPermission(dataset).test() | |||
| form = api.validate(ResourceForm) | |||
| resource = Resource() | |||
| form = api.validate(ResourceFormWithoutId) | |||
| @@ -104,6 +104,10 @@ class ResourceForm(BaseResourceForm): | |||
| id = fields.UUIDField() | |||
|
|
|||
|
|
|||
| class ResourceFormWithoutId(BaseResourceForm): | |||
There was a problem hiding this comment.
I'm confused we use ResourceFormWithoutId in DatasetForm?
There was a problem hiding this comment.
Because otherwise we don't have the ID for the reorder for exemple…
| resources_ids = set() | ||
| for resource in self.resources: | ||
| if resource.id in resources_ids: | ||
| raise MongoEngineValidationError( | ||
| f"Duplicate resource ID {resource.id} in dataset #{self.id}." | ||
| ) | ||
| resources_ids.add(resource.id) |
There was a problem hiding this comment.
Could we do something like?
if len(set(self.resources)) != len(res.id for res in self.resources):
raise...
There was a problem hiding this comment.
I think it's slower since there is two iteration but yes it's shorter…
udata/core/dataset/models.py
Outdated
| @@ -335,7 +335,7 @@ def to_mongo(self, *args, **kwargs): | |||
|
|
|||
|
|
|||
| class ResourceMixin(object): | |||
| id = db.AutoUUIDField(primary_key=True) | |||
| id = db.AutoUUIDField(primary_key=True, unique=True) | |||
There was a problem hiding this comment.
Do we need the manual check if we already have unique=True?
There was a problem hiding this comment.
Yes I can remove this, I didn't find a way to make it work
There was a problem hiding this comment.
I removed it. Adding it fails a lot of tests I don't know why… https://app.circleci.com/pipelines/github/opendatateam/udata/5863/workflows/ee92a1f9-786c-42dc-a1e9-dc5b5a3be992/jobs/33495?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-checks-link&utm_content=summary
| @@ -638,6 +638,14 @@ def clean(self): | |||
| if self.frequency in LEGACY_FREQUENCIES: | |||
| self.frequency = LEGACY_FREQUENCIES[self.frequency] | |||
|
|
|||
| resources_ids = set() | |||
There was a problem hiding this comment.
These manual checks only care about the current dataset though?
There was a problem hiding this comment.
Yes we cannot check for all resources in different dataset (but I think resources are always scoped by the dataset ID so it shouldn't be a problem, no?)
No description provided.