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(inline, expand): Only accept on structures, assocs or table aliases #551

Merged
merged 6 commits into from
Apr 9, 2024

Conversation

patricebender
Copy link
Member

@patricebender patricebender commented Mar 26, 2024

an expand or inline on a many type has no defined behavior, yet.
→ as of now this is an error because the path cant be resolved (expand / inline without .elements)

we should reject it with a proper error, just as the compiler does for static views.

an `expand` on a `many` type has no defined behavior, yet.

we should reject it, just as the compiler does for static views.
@@ -300,6 +300,9 @@ entity SoccerPlayers {
key jerseyNumber: Integer;
name: String;
team: Association to SoccerTeams;
emails: many {
Copy link
Contributor

Choose a reason for hiding this comment

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

afaik, collection valued properties are not queried with expand but as a regular column

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, this PR aligns the runtime with the compiler behavior. I would say, this change is fine as-is because it definetly improves the current error situation. If we come up with some meaningful semantics I'd be happy to implement it 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

the error msg however is not clear to me.

"Unexpected “expand” on “emails”; can only be used after a reference to a structure, association or table alias does not tell me that I should use it as a regular column instead of expanding it.

Copy link
Member Author

@patricebender patricebender Mar 26, 2024

Choose a reason for hiding this comment

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

How about Expecting a reference to a structure, association, or table alias before using '{ ‹expand› }'.

→ The error message which comes with this change tries to tell the user that the expand on the arrayed element is unexpected. In the second part of the message, I try to give guidance on which columns the expand could be used.

In the compiler the error message is very similar:

Error[def-unexpected-nested-proj]: Unexpected ‘{ ‹expand› }’; can only be used after a reference to a structure, association or table alias
    |
  e.cds:23.16-25.6, at entity:“A”/column:“manyStruct”
    |
 23 |     manyStruct {
 24 |       leaf
 25 |     }

Navigating into an arrayed structure is not possible, yet. At least its not supported by the compiler SQL Backend.

entity Authors {
  key ID         : Int16;
      name       : String(111);
      manyStruct : many {
        leaf   : Integer;
        sub    : {
          leaf : Integer;
        }
      };
}

will in the end only be:

-- generated by cds-compiler version 4.8.1
CREATE TABLE Authors (
  ID SMALLINT NOT NULL,
  name NVARCHAR(111),
  manyStruct NCLOB, -- leafs are all gone
  PRIMARY KEY(ID)
);

if you try to navigate into the struct:

Error[ref-unexpected-many-navigation]: Unexpected navigation into arrayed structure
    |
  e.cds:23:5, at entity:“A”/select/column:“leaf”
    |
 23 |     manyStruct.leaf
    |     ^

@patricebender patricebender enabled auto-merge (squash) March 26, 2024 14:41
@patricebender
Copy link
Member Author

@stewsk please have a look

@patricebender patricebender changed the title fix(expand): Only accept on structures, assocs or table aliases fix(nested projections): Only accept on structures, assocs or table aliases Apr 9, 2024
@patricebender patricebender changed the title fix(nested projections): Only accept on structures, assocs or table aliases fix(inline, expand): Only accept on structures, assocs or table aliases Apr 9, 2024
@patricebender patricebender merged commit 3248512 into main Apr 9, 2024
5 checks passed
@patricebender patricebender deleted the patrice/expand branch April 9, 2024 12:59
@cap-bots cap-bots mentioned this pull request Apr 9, 2024
johannes-vogel added a commit that referenced this pull request Apr 12, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>db-service: 1.8.0</summary>

##
[1.8.0](db-service-v1.7.0...db-service-v1.8.0)
(2024-04-12)


### Added

* Odata built-in query functions
([#558](#558))
([6e63367](6e63367))
* support HANA stored procedures
([#542](#542))
([52a00a0](52a00a0))


### Fixed

* **`expand`:** Only accept on structures, assocs or table aliases
([#551](#551))
([3248512](3248512))
* **`order by`:** for localized sorting, prepend table alias
([#546](#546))
([a273a92](a273a92))
* etag with stream_compat
([#562](#562))
([b0a3a41](b0a3a41))
* exclude `cds.LargeBinary` from wildcard expansion
([#577](#577))
([6661d63](6661d63))
* Reduce insert queries for deep update
([#568](#568))
([55e5114](55e5114))
* Reduced count query complexity when possible
([#553](#553))
([3331f02](3331f02))
</details>

<details><summary>postgres: 1.7.0</summary>

##
[1.7.0](postgres-v1.6.0...postgres-v1.7.0)
(2024-04-12)


### Added

* Odata built-in query functions
([#558](#558))
([6e63367](6e63367))
</details>

<details><summary>hana: 0.2.0</summary>

##
[0.2.0](hana-v0.1.0...hana-v0.2.0)
(2024-04-12)


### Added

* Odata built-in query functions
([#558](#558))
([6e63367](6e63367))
* support HANA stored procedures
([#542](#542))
([52a00a0](52a00a0))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: Johannes Vogel <[email protected]>
@cap-bots cap-bots mentioned this pull request Jul 18, 2024
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.

3 participants