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

feat: Persist assert_integrity feature #1032

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

feat: Persist assert_integrity feature #1032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f7935d4
Persist assert_integrity feature
daogrady Feb 18, 2025
64d4571
Carry over changes
daogrady Feb 18, 2025
14eaeb1
Merge branch 'main' into fix/persist-assert_integrity
daogrady Feb 19, 2025
6b70038
Shorten
daogrady Feb 19, 2025
29eb6d1
Make more robust towards existing feature blocks
daogrady Feb 19, 2025
b43acd3
Add test
daogrady Feb 19, 2025
b3db6fd
Add todo
daogrady Feb 19, 2025
8d2fb6d
Finalise test
daogrady Feb 19, 2025
19c35b1
Merge branch 'main' into fix/persist-assert_integrity
daogrady Feb 19, 2025
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
22 changes: 17 additions & 5 deletions postgres/cds-plugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,23 @@ cds.build?.register?.('postgres', class PostgresBuildPlugin extends cds.build.Pl
if (fs.existsSync(path.join(this.task.src, 'package.json'))) {
promises.push(this.copy(path.join(this.task.src, 'package.json')).to('package.json'))
} else {
promises.push(
this.write({
dependencies: { '@sap/cds': '^8', '@cap-js/postgres': '^1' },
scripts: { start: 'cds-deploy' },
}).to('package.json'),
const packageJson = {
dependencies: {
'@sap/cds': '^8',
'@cap-js/postgres': '^1'
},
scripts: {
start: 'cds-deploy'
}
}
const assertIntegrity = cds.env?.features?.assert_integrity
if (assertIntegrity) {
packageJson.cds ??= {}
packageJson.cds.features ??= {}
packageJson.cds.features.assert_integrity = assertIntegrity
Copy link
Member

Choose a reason for hiding this comment

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

We could already prepare for other supported feature flags in a generic way, because once another feature flag will be added, this should be done anyway. Something like

Copy link
Author

@daogrady daogrady Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I actually started with a generic implementation that would take an arbitrary path into the cds.env, prepare all parent objects in package.json based on the type of what cds.env dictated and finally copied over all appropriate values. And before I knew it, I had a 200 loc function, that somewhat paled in comparison to this 5 line solution for the problem at hand. 😃
I therefore suggest to keep it simple as long as we are dealing with only one value and generalise it later, if we need to carry over more settings.

Copy link
Member

Choose a reason for hiding this comment

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

I see. However, I am not really happy with just fixing the passthrough for this one feature. I can imagine other feature flags which are important for a proper build, such as: naming modes (quoted or plain names?), standard database function mappings, …

@stewsk @johannes-vogel @chgeo we have the issue that build relevant options are not per default copied over to the postgres deployer app (found in gen/pg) do you have some idea how to properly copy over build relevant options (which are those?) to the deployer app? If we don't do that, an mbt build && cf deploy will lose all SQL relevant features defined in the app which e.g. has been the case for cds.features.assert_integrity in #1020

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that there's a dedicated section for deployment relevant things. They are spread over different sections.

Copy link
Member

Choose a reason for hiding this comment

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

Would it then in your opinion be sufficient to have a kind of allow-list approach, where we selectively copy over known build relevant options?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if an allow list makes sense if we do not know the options. How is that solved for the hana deployer?

Copy link
Member

Choose a reason for hiding this comment

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

during build for HANA the actual db artifacts are already created with all features found in the cds env. Upon a deployment the hdi artifacts already have the desired features baked in. In contrast to that, for postgres, only a inferred CSN is generated during build. Which does not contain any DB specific transformations, hence lacking the DB features set in the cds env. Upon deployment, the gen/pg/csn.json is transformed to the actual SQL schema. If the deployment is initiated in the gen/pg/ folder, the root app cds env is ignored in favor of the the cds env of the deployer app (gen/pg/package.json) which is empty, invalidating all features set in the apps root.

}
promises.push(
this.write(packageJson).to('package.json')
)
}
promises.push(this.write(cds.compile.to.json(model)).to(path.join('db', 'csn.json')))
Expand Down
11 changes: 10 additions & 1 deletion postgres/test/cds-build.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ const cds = require('../../test/cds.js')

const workDir = path.join(__dirname, 'tiny-sample')
const genDir = path.join(workDir, 'gen')
const dbDest = path.join(genDir, 'pg/db')
const pgDest = path.join(genDir, 'pg')
const dbDest = path.join(pgDest, 'db')

// delete the generated folder after each test
afterEach(() => {
Expand All @@ -27,4 +28,12 @@ describe('cds build plugin', () => {
execSync('npx cds build --production', { cwd: workDir })
expect(fs.existsSync(path.join(dbDest, 'csn.json'))).to.be.true
})

test('should retain assert_integrity setting', () => {
execSync('npx cds build --production', { cwd: workDir })
const packageJson = require(path.join(pgDest, 'package.json'))
expect(packageJson.cds?.features?.assert_integrity).to.equal('db')
const ddl = String(execSync('npx cds deploy --dry', { cwd: workDir }))
expect(ddl).to.contain('REFERENCES')
})
})
5 changes: 5 additions & 0 deletions postgres/test/tiny-sample/db/schema.cds
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,9 @@ entity Books {
key ID : Integer;
title : String;
stock : Integer;
author : Association to Authors;
}

entity Authors {
key ID : Integer;
}
5 changes: 5 additions & 0 deletions postgres/test/tiny-sample/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,10 @@
"description": "A simple CAP project, to test the build plugin",
"dependencies": {
"@cap-js/postgres": "../../."
},
"cds": {
"features": {
"assert_integrity": "db"
}
}
}
Loading