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

fix: Prioritize element name over implicit alias in path resolution #1071

Closed
wants to merge 3 commits into from

Conversation

patricebender
Copy link
Member

@patricebender patricebender commented Mar 11, 2025

For recursive entity structures like:

entity Genres {
  ID:   Integer;
  name: String;
  parent: Association to Genres;
}

an OData request such as:

http://localhost:4004/odata/v4/CatalogService/Genres(42)/dependsOn?$orderby=parent/name

leads to a cqn like select from Genres:parent order by parent.name

this would previously resolve the first path step in parent.name as implicit table alias parent, even though there is an element (the association parent)with the same name. With this change, the first reference of any multi-segment path is resolved using the following order:

  1. Explicit table alias: If provided (e.g., in select from Genres:parent as parent order by parent.name), use the explicit alias.
  2. Element reference: If no explicit alias exists, resolve the element (e.g., parent resolves to Genres:parent).
  3. Implicit table alias: Fallback to the implicit alias.
  4. Error: Throw an error if none of the above can be matched.

This update maintains compatibility with standard SQL implicit alias behavior while ensuring that element names take precedence where applicable.

For recursive entity structures like:

```cds
entity Genres {
  ID:   Integer;
  name: String;
  parent: Association to Genres;
}
```

an OData request such as:

`http://localhost:4004/odata/v4/CatalogService/Genres(42)/dependsOn?$orderby=parent/name`

could previously resolve the path "parent.name" ambiguously. With this change, the first reference of any multi-segment path is resolved using the following order:

1. Explicit table alias: If provided (e.g., in `select from Genres:parent as parent order by parent.name`), use the explicit alias.
2. Element reference: If no explicit alias exists, resolve the element (e.g., `parent` resolves to `Genres:parent`).
3. Implicit table alias: Fallback to the implicit alias.
4. Error: Throw an error if none of the above can be matched.

This update maintains compatibility with standard SQL implicit alias behavior while ensuring that element names take precedence where applicable.
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 did a few quick tests in cds repl -r @capire/bookshop, with this added to bookshop/db/schema.cds:

@cds.persistence.skip: false
entity parent as select ID, name, parent from Genres;

Works as expected:

await SELECT `ID, name from Genres order by name`
await SELECT `ID, name as genres from Genres order by name`
await SELECT `ID, name, parent.name from Genres order by name`
await SELECT `ID, name, parent.name from Genres order by parent.name`
await SELECT `ID, name, parent.name from Genres as g order by parent.name`
await SELECT `ID, name from parent order by name`

Fails with errors:

await SELECT `ID, name, parent.name from parent order by parent.name`
await SELECT `ID, name, parent.name from parent` 
await SELECT `ID, name, parent.name from Genres:parent` 
await SELECT `ID, name, parent.name from Genres as parent`
await SELECT `ID, name from parent as g order by parent.name`

Works as fixes for the above, but the above should work ootb:

await SELECT `ID, name, parent.name as parent_name from parent order by parent.name`
await SELECT `ID, name, parent.name as parent_name from parent`
await SELECT `ID, name, parent.name as parent_name from Genres:parent` 
await SELECT `ID, name, parent.name as parent_name from Genres as parent`

Note: as parent_name is anyways the default I assumed we apply also for the failing ones?

Works, but seems wrong precedence:

await SELECT `ID, name from parent order by parent.name`

→ seems to do the same as:

await SELECT `ID, name from parent order by name`

Didn't we say that only explicit aliases have precedence? → i.e.:

await SELECT `ID, name from Genres as parent order by parent.name`

@patricebender
Copy link
Member Author

patricebender commented Mar 11, 2025

Thanks for your review!

This one is a good catch:

await SELECT `ID, name, parent.name from Genres:parent`

→ should not fail, added a test and fixed 👍

rest will follow

@patricebender patricebender requested a review from danjoa March 11, 2025 18:36
@patricebender
Copy link
Member Author

await SELECT ID, name, parent.name from parent order by parent.name

there is no element parent in the projection, hence the parent refers to the implicit table alias. Duplicate definition of element name rightfully thrown.

same for

await SELECT `ID, name, parent.name from parent`

await SELECT ID, name, parent.name from Genres as parent

here rule number 1. kicks in, the explicit table alias ha s precedence over element Genres:parent hence you select name twice. Duplicate definition of element name rightfully thrown.

await SELECT ID, name from parent as g order by parent.name

there is no parent in parent.elements (the projection does not expose the association.) Also, explicit table alias g is assigned, hence parent.name can't be resolved. "parent" not found in the elements of "sap.capire.bookshop.parent" rightfully thrown.

await SELECT ID, name from parent order by parent.name

"Works, but seems wrong precedence:" → No. There is no element parent in parent.elements (the projection does not expose the association) so there is only one name.

@danjoa please check

@danjoa
Copy link
Contributor

danjoa commented Mar 17, 2025

Last update from spec meeting:

  • Precedence rules stay as decided recently
  • Implicit aliases should be always $Author and alike → as before but prefixed with $breaking change for Node.js
  • If developers want to use aliases in where, order by, ... clauses they should specify explicit aliases
  • If they used JOIN in their queries we can stay with implicit aliases w/o $ → as of today

@patricebender
Copy link
Member Author

patricebender commented Mar 17, 2025

Last update from spec meeting:

  • Precedence rules stay as decided recently
  • Implicit aliases should be always $Author and alike → as before but prefixed with $breaking change for Node.js
  • If developers want to use aliases in where, order by, ... clauses they should specify explicit aliases
  • If they used JOIN in their queries we can stay with implicit aliases w/o $ → as of today

got it 👍 already started migrating ALL the tests last week: #1082

will take me some time to finish this up

@patricebender patricebender deleted the patrice/naming branch March 17, 2025 10:04
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