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: joins without columns are rejected #535

Merged
merged 8 commits into from
Mar 20, 2024
Merged

fix: joins without columns are rejected #535

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3857593
fix: joins without columns are rejected
johannes-vogel Mar 18, 2024
e415690
reject on hana as well
johannes-vogel Mar 18, 2024
87d5e99
move check to cqn2sql
johannes-vogel Mar 19, 2024
32eb15b
add test without conflicts in join
johannes-vogel Mar 19, 2024
70422ae
also reject on hana
johannes-vogel Mar 19, 2024
c5dafe8
Update test/scenarios/bookshop/read.test.js
johannes-vogel Mar 20, 2024
e0aa6a4
Update test/scenarios/bookshop/read.test.js
johannes-vogel Mar 20, 2024
d42e61c
Merge branch 'main' into fix--joins-without-columns
johannes-vogel Mar 20, 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
5 changes: 5 additions & 0 deletions db-service/lib/SQLService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,11 @@ class SQLService extends DatabaseService {
* @type {Handler}
*/
async onSELECT({ query, data }) {
if (query.SELECT.from?.join && !query.SELECT.columns) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the join is not on root level ?

SELECT ID FROM (
  SELECT * FROM
  Books
  LEFT JOIN
  Authors
  ON
  Books.author_ID = Authors.ID
)

Maybe it makes sense to move it into cqn2sql (code). Which will reject it on all levels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll double check

throw new Error('CQN query using joins must specify the selected columns.')
}


if (!query.target) {
try { this.infer(query) } catch (e) { /**/ }
}
Expand Down
17 changes: 17 additions & 0 deletions test/scenarios/bookshop/read.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,23 @@ describe('Bookshop - Read', () => {
expect((await cds.db.run(query)).count).to.be.eq(2)
})

it('joins without columns are rejected', async () => {
const query = {
SELECT: {
from: {
join: 'inner',
args: [
{ ref: ['sap.capire.bookshop.Books'], as: 'b' },
{ ref: ['sap.capire.bookshop.Authors'], as: 'a' },
],
on: [{ ref: ['a', 'ID'] }, '=', { ref: ['b', 'author_ID'] }],
},
},
}

expect(cds.db.run(query)).to.be.rejectedWith(/joins must specify the selected columns/)
})

test('Delete Book', async () => {
const res = await DELETE('/admin/Books(271)', admin)
expect(res.status).to.be.eq(204)
Expand Down
Loading