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

Shouldn't the scope of the unique validation be the resource, instead of the collection? #1291

Closed
arnauorriols opened this issue Jun 27, 2019 · 3 comments · Fixed by #1292
Closed
Milestone

Comments

@arnauorriols
Copy link
Contributor

@arnauorriols arnauorriols commented Jun 27, 2019

After this change, the unique attributes must be unique for the entire MongoDB collection. Previously, they had to be unique only for the API resource. This becomes a problem when the datasource is configured with a filter and you have multiple resources sharing the same database collection.

At first we thought this was just a bug. However, in the mentioned commit a comment is introduced indicating that the change is in fact intentional:

we perform the check on the native mongo driver (and not on
app.data.find_one()) because in this case we don't want the usual
(for eve) query injection to interfere with this validation. We
are still operating within eve's mongo namespace anyway.

However, I fail to understand its motivation. Can you elaborate?

We don't use the soft-delete feature ourselves, but looking at the code, seems like it should also be affected (ie soft-deleted documents will be taken into account when validating the uniqueness of an attribute).

@arnauorriols
Copy link
Contributor Author

@arnauorriols arnauorriols commented Jun 30, 2019

Ok, so after reading the docs and the tests more carefully, I think I understand now the source of the conflict.

There are different and conflicting use cases for the datasource.filter feature:

a) Have N resources, where one of them is the canonical one, and the rest are subsets of the canonical one. Kind of named filters, or database views.
b) Have N resources, each of them canonical, that for some reason you want to keep in the same database collection. One possible reason could be that you want to have another (readonly) resource that retrieves the documents of all the other resources.

I think both use cases are valid, so I propose to add another unique_xxx validation rule to discriminate them. As this change was released quite a while ago (we just upgraded now from 0.5.3, sorry!), I think unique should remain as it is now, and the new unique validation rule should be unique_within_resource. I've opened #1292 with the suggestion.

What do you think?

@nicolaiarocci nicolaiarocci added this to the 0.9.3 milestone Jul 2, 2019
@nicolaiarocci
Copy link
Member

@nicolaiarocci nicolaiarocci commented Jul 2, 2019

I think you did an excellent job with #1292

@arnauorriols
Copy link
Contributor Author

@arnauorriols arnauorriols commented Jul 8, 2019

Cool. When are you planning on merging it?

@nicolaiarocci nicolaiarocci removed this from the 0.9.3 milestone Jul 11, 2019
@nicolaiarocci nicolaiarocci added this to the 0.10 milestone Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants