-
Notifications
You must be signed in to change notification settings - Fork 0
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
Support PUT/PATCH/DELETE requests on resources with a translated ID field #154
Conversation
d3be662
to
9138787
Compare
9138787
to
f7e0ab2
Compare
test/package.json
Outdated
@@ -0,0 +1,4 @@ | |||
{ | |||
"description": "Without this nodejs >=22.7.0 will see the module syntax in the tests, assume incorectly that we are using ESM, and fail once it tries run the imports", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
W/o this running the tests on node 22.7 fails with:
(node:93162) [MODULE_TYPELESS_PACKAGE_JSON] Warning: file:///<path>/odata-to-abstract-sql/test/chai-sql.js parsed as an ES module because module syntax was detected; to avoid the performance penalty of syntax detection, add "type": "module" to /<path>/odata-to-abstract-sql/test/package.json
(Use `node --trace-warnings ...` to show where the warning was created)
Exception during run: file:///<path>/odata-to-abstract-sql/test/chai-sql.js:5
import { createTranslator } from '@balena/lf-to-abstract-sql';
^^^^^^^^^^^^^^^^
SyntaxError: Named export 'createTranslator' not found. The requested module '@balena/lf-to-abstract-sql' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:
import pkg from '@balena/lf-to-abstract-sql';
const { createTranslator } = pkg;
at ModuleJob._instantiate (node:internal/modules/esm/module_job:171:21)
at async ModuleJob.run (node:internal/modules/esm/module_job:254:5)
at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:482:26)
at async formattedImport (/<path>/odata-to-abstract-sql/node_modules/mocha/lib/nodejs/esm-utils.js:9:14)
at async Object.exports.requireOrImport (/<path>/odata-to-abstract-sql/node_modules/mocha/lib/nodejs/esm-utils.js:42:28)
at async Object.exports.loadFilesAsync (/<path>/odata-to-abstract-sql/node_modules/mocha/lib/nodejs/esm-utils.js:100:20)
at async singleRun (/<path>/odata-to-abstract-sql/node_modules/mocha/lib/cli/run-helpers.js:162:3)
at async Object.exports.handler (/<path>/odata-to-abstract-sql/node_modules/mocha/lib/cli/run.js:375:5)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the real issue is that a type
should be present in the top-level package.json which for this repo is indeed commonjs, I've fixed that and most of the lint warnings in #155 so rebasing on top of that should remove a lot of the noise from this PR (mainly due to the linting warnings)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I would swear that I also tried adding the type to the top level package.json... Well we indeed don't need this now 👍
src/odata-to-abstract-sql.ts
Outdated
} else { | ||
tableOrSubqueryNode = fromNode; | ||
if (!isTableNode(tableOrSubqueryNode)) { | ||
throw new Error(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get a better error message here, if nothing else having a placeholder that can be searched for is probably helpful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ends up that was a leftover when I was trying to avoid the if (aliasNode == null)
check.
Atm it's not needed and is already checked a bit lower at:
if (!isSelectQueryNode(tableOrSubqueryNode)) {
throw new Error(
`Adding a nested field select to a subquery containing a ${tableOrSubqueryNode[0]} is not supported`,
);
}
src/odata-to-abstract-sql.ts
Outdated
method === 'PATCH' | ||
? '$updateid' | ||
: method === 'DELETE' | ||
? '$deleteid' | ||
: '$upsertid', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might make more sense to just use eg $modifyid
to avoid branching on the name, since I don't think there's ever a point where we actually need to differentiate between update/delete/upsert in the naming since they never intermix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I wanted from the first place :) 🎉
f7e0ab2
to
f18fd8b
Compare
src/odata-to-abstract-sql.ts
Outdated
fieldNameAlias, | ||
); | ||
selectNode.push(['ReferencedField', aliasName, fieldNameAlias]); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return; |
…ield Change-type: minor
f18fd8b
to
65b0676
Compare
Depends-on: balena-io-modules/odata-to-abstract-sql#154 Change-type: patch See: https://balena.fibery.io/Work/Project/Server-side-pagination-for-devices-Cycle-4-673 See: https://balena.fibery.io/Work/Task/odata-to-abstract-sql-Support-PUT-PATCH-DELETE-requests-on-resources-with-a-translated-ID-field-2089
Depends-on: balena-io-modules/odata-to-abstract-sql#154 Change-type: patch See: https://balena.fibery.io/Work/Project/Server-side-pagination-for-devices-Cycle-4-673 See: https://balena.fibery.io/Work/Task/odata-to-abstract-sql-Support-PUT-PATCH-DELETE-requests-on-resources-with-a-translated-ID-field-2089
Depends-on: balena-io-modules/odata-to-abstract-sql#154 Change-type: patch See: https://balena.fibery.io/Work/Project/Server-side-pagination-for-devices-Cycle-4-673 See: https://balena.fibery.io/Work/Task/odata-to-abstract-sql-Support-PUT-PATCH-DELETE-requests-on-resources-with-a-translated-ID-field-2089
Depends-on: balena-io-modules/odata-to-abstract-sql#154 Change-type: patch See: https://balena.fibery.io/Work/Project/Server-side-pagination-for-devices-Cycle-4-673 See: https://balena.fibery.io/Work/Task/odata-to-abstract-sql-Support-PUT-PATCH-DELETE-requests-on-resources-with-a-translated-ID-field-2089
Depends-on: balena-io-modules/odata-to-abstract-sql#154 Change-type: patch See: https://balena.fibery.io/Work/Project/Server-side-pagination-for-devices-Cycle-4-673 See: https://balena.fibery.io/Work/Task/odata-to-abstract-sql-Support-PUT-PATCH-DELETE-requests-on-resources-with-a-translated-ID-field-2089
Depends-on: balena-io-modules/odata-to-abstract-sql#154 Change-type: patch See: https://balena.fibery.io/Work/Project/Server-side-pagination-for-devices-Cycle-4-673 See: https://balena.fibery.io/Work/Task/odata-to-abstract-sql-Support-PUT-PATCH-DELETE-requests-on-resources-with-a-translated-ID-field-2089
Depends-on: balena-io-modules/odata-to-abstract-sql#154 Change-type: patch See: https://balena.fibery.io/Work/Project/Server-side-pagination-for-devices-Cycle-4-673 See: https://balena.fibery.io/Work/Task/odata-to-abstract-sql-Support-PUT-PATCH-DELETE-requests-on-resources-with-a-translated-ID-field-2089
Change-type: minor
See: https://balena.fibery.io/Work/Project/Server-side-pagination-for-devices-Cycle-4-673
See: https://balena.fibery.io/Work/Task/odata-to-abstract-sql-Support-PUT-PATCH-DELETE-requests-on-resources-with-a-translated-ID-field-2089
See: https://balena.zulipchat.com/#narrow/stream/403752-channel.2Fsupport-help/topic/Error.20500.20on.20should.20be.20running.20release/near/464744246
See: https://gist.github.com/thgreasi/7cf6eba5634e813a7e34a0832724de36