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

It's possible to create reference targetting non-existing evitaDB entity type #162

Closed
novoj opened this issue Jun 30, 2023 · 6 comments · Fixed by #361
Closed

It's possible to create reference targetting non-existing evitaDB entity type #162

novoj opened this issue Jun 30, 2023 · 6 comments · Fixed by #361
Assignees
Labels
bug Something isn't working

Comments

@novoj
Copy link
Collaborator

novoj commented Jun 30, 2023

This should clearly not be possible.

@novoj novoj added the bug Something isn't working label Jun 30, 2023
@novoj novoj added this to the Alpha milestone Jun 30, 2023
@novoj novoj self-assigned this Jun 30, 2023
@novoj
Copy link
Collaborator Author

novoj commented Dec 5, 2023

Unfortunately, this issue was created in a hurry without a proper description of what it really means. Closed for now - if it's a problem, it should come up again.

@novoj novoj closed this as completed Dec 5, 2023
@novoj novoj removed this from the Alpha milestone Dec 5, 2023
@lukashornych
Copy link
Collaborator

@novoj I think it was created when I was writing the "Write data" docs chapter, where it allowed me to create reference to evitaDB-managed brand and category entities but these entities where not created yet. I think in GQL it wasn't possible because I was internally looking for schema of that entities, but directly in Java it was possible.

@novoj novoj reopened this Dec 5, 2023
@novoj
Copy link
Collaborator Author

novoj commented Dec 5, 2023

Thank you! I will fix it today then.

novoj added a commit that referenced this issue Dec 5, 2023
Former implementation allowed to create reference targeting non-existing evitaDB entity type.
@novoj novoj linked a pull request Dec 5, 2023 that will close this issue
@novoj novoj closed this as completed in #361 Dec 5, 2023
novoj added a commit that referenced this issue Dec 5, 2023
…erence-targetting-non-existing-evitadb-entity-type

fix(#162): add validation for managed entity references
@novoj novoj reopened this Dec 5, 2023
@novoj
Copy link
Collaborator Author

novoj commented Dec 5, 2023

This implementation broke a lot of tests and I'm afraid it would break current clients as well. I think we need to postpone validation and allow multiple schemas to be defined in bulk to avoid unnecessary complexity on the client side while maintaining model consistency.

Current use case:

Product -> References Category
category -> references products (related products specified at category level)

It's a circular dependency, but it's a real use case that we need to support. The only situation to avoid the exception would be

  1. create category
  2. create product that references category
  3. change category to reference related products

This requires quite complicated setup on the client side, which now uses the ClassAnalyzer facility to set up the schema.

So a new suggestion would be to validate the schema not in the mutation, but at the end of the updateSchema method. In io.evitadb.core.EvitaSession#updateCatalogSchema method the validation will be done after all mutations have been processed. This would allow to set up circular dependent updates in one go and still pass the validation.

@lukashornych
Copy link
Collaborator

I agree with this solution. I've checked the GraphQL API where the problem occurs and the exception is thrown when the GraphQL API is being regenerated after schema update, so this should play well with your idea.

novoj added a commit that referenced this issue Dec 5, 2023
novoj added a commit that referenced this issue Dec 6, 2023
Reworked original implementation which broke a lot of tests and I'm afraid it would break current clients as well. I think we need to postpone validation and allow multiple schemas to be defined in bulk to avoid unnecessary complexity on the client side while maintaining model consistency.

Current use case:

Product -> References Category
category -> references products (related products specified at category level)

It's a circular dependency, but it's a real use case that we need to support. The only situation to avoid the exception would be

1. create category
2. create product that references category
3. change category to reference related products

This requires quite complicated setup on the client side, which now uses the `ClassAnalyzer` facility to set up the schema.

So a new suggestion would be to validate the schema not in the mutation, but at the end of the updateSchema method. In `io.evitadb.core.EvitaSession#updateCatalogSchema` method the validation will be done after all mutations have been processed. This would allow to set up circular dependent updates in one go and still pass the validation.
@novoj novoj closed this as completed Dec 6, 2023
novoj added a commit that referenced this issue Dec 6, 2023
Cleaning - two interfaces joined together.
@novoj
Copy link
Collaborator Author

novoj commented Dec 7, 2023

Few grey hairs later ... it's finally done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants