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

chore: unify physical elements check #408

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

BobdenOs
Copy link
Contributor

Add an exists(<element>) function into cqn2sql to unify checking whether an element exists in the database.

element.virtual // virtual does not exist on the database
element.value // calculated column cannot be written into
element.isAssociation // associations are meta columns that cannot be addressed

@BobdenOs
Copy link
Contributor Author

this change still breaks @sap/cds tests.

Copy link
Contributor

@danjoa danjoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in most cases we allow !(c in elements) to read or feed native db columns, not in the model → would only reject in strict mode.

In addition we need to follow up on the isAssociation case marked as revisit, which breaks a test in @sap/cds if we always skip these.

@BobdenOs
Copy link
Contributor Author

We do allow writing into elements that don't exists inside the model. The distinction that this PR is trying to address is the difference between the model and the actual tables on the database. For us to be able to allow users to provide associations and virtual fields it is required that we don't include these values inside the SQL statements or the database would throw.

Additionally for the UPSERT case with the isAssociation check it should be allowed to provide data inside an association column, but currently it is not allowed to do deep UPSERT queries. Which is the test case that is failing. If we define that with the new databases it is allowed to do deep UPSERT it would mean that the current expectation is wrong. As currently the UPSERT implementation is wrong when providing data inside an association column.

@danjoa
Copy link
Contributor

danjoa commented Jan 29, 2024

currently it is not allowed to do deep UPSERT queries. Which is the test case that is failing

Yes, and that test is strangely written in a way that it expects associations to pass through and crash in the database → if we are sure that this test is written badly and fix it, I think we can filter associations always.

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 this pull request may close these issues.

2 participants