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.

Sign up for GitHub

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
Merged

fix(inline, expand): Only accept on structures, assocs or table aliases #551

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b47b756
fix(`expand`): Only accept on structures, assocs or table aliases
patricebender Mar 26, 2024
7421aaa
Merge branch 'main' into patrice/expand
patricebender Mar 26, 2024
b307dc5
Merge branch 'main' into patrice/expand
patricebender Mar 28, 2024
819a975
Merge branch 'main' into patrice/expand
patricebender Apr 9, 2024
e844bc6
same goes for inline
patricebender Apr 9, 2024
cee4d6e
Merge branch 'main' into patrice/expand
patricebender Apr 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 16 additions & 13 deletions db-service/lib/infer/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -795,19 +795,22 @@ function infer(originalQuery, model) {
? new cds.struct({ elements: inferredExpandSubquery.elements })
: new cds.array({ items: new cds.struct({ elements: inferredExpandSubquery.elements }) })
return Object.defineProperty(res, '$assocExpand', { value: true })
} // struct
let elements = {}
expand.forEach(e => {
if (e === '*') {
elements = { ...elements, ...$leafLink.definition.elements }
} else {
inferQueryElement(e, false, $leafLink, { inExpr: true, inNestedProjection: true })
if (e.expand) elements[e.as || e.flatName] = resolveExpand(e)
if (e.inline) elements = { ...elements, ...resolveInline(e) }
else elements[e.as || e.flatName] = e.$refLinks ? e.$refLinks[e.$refLinks.length - 1].definition : e
}
})
return new cds.struct({ elements })
} else if ($leafLink.definition.elements) {
let elements = {}
expand.forEach(e => {
if (e === '*') {
elements = { ...elements, ...$leafLink.definition.elements }
} else {
inferQueryElement(e, false, $leafLink, { inExpr: true, inNestedProjection: true })
if (e.expand) elements[e.as || e.flatName] = resolveExpand(e)
if (e.inline) elements = { ...elements, ...resolveInline(e) }
else elements[e.as || e.flatName] = e.$refLinks ? e.$refLinks[e.$refLinks.length - 1].definition : e
}
})
return new cds.struct({ elements })
} else {
throw new Error(`Unexpected “expand” on “${col.ref.map(idOnly)}”; can only be used after a reference to a structure, association or table alias`)
}
}

function stepNotFoundInPredecessor(step, def) {
Expand Down
3 changes: 3 additions & 0 deletions db-service/test/bookshop/db/schema.cds
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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
    |     ^

address: String;
}
}

entity TestPublisher {
Expand Down
11 changes: 11 additions & 0 deletions db-service/test/cds-infer/negative.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,17 @@ describe('negative', () => {
),
).to.throw(/"title" not found in the elements of "bookshop.Authors"/)
})

it('expand on `.items` not possible', () => {
expect(() => _inferred(CQL`SELECT from bookshop.SoccerPlayers { name, emails { address } }`, model)).to.throw(
"Unexpected “expand” on “emails”; can only be used after a reference to a structure, association or table alias"
)
})
it('expand on scalar not possible', () => {
expect(() => _inferred(CQL`SELECT from bookshop.SoccerPlayers { name { address } }`, model)).to.throw(
"Unexpected “expand” on “name”; can only be used after a reference to a structure, association or table alias"
)
})
})

describe('infix filters', () => {
Expand Down