Validate names#129
Conversation
SummaryThe following content is AI-generated and provides a summary of the pull request: Add Notification Type Compilation from CDS Model with Element Name ValidationNew Feature✨ Introduces a new Changes
PR Bot InformationVersion:
|
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR introduces a solid new lib/compile.js module that derives notification types directly from the CDS model and adds 128-character element name validation. There are a few issues to address: a potential runtime TypeError in resolveEnum(...).toUpperCase() when the channel value isn't a string, an unhandled promise rejection risk in the served async callback if cds.load or notificationTypesFromModel throws, and a missing top-level const cds = require('@sap/cds') in the unit test file that will cause a ReferenceError in the subtitle i18n test.
PR Bot Information
Version: 1.20.51
- File Content Strategy: Full file content
- Correlation ID:
dffe0143-1e88-46b8-a603-a0cccf70f16f - LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.opened
| if (def['@notification.deliveryChannels']?.length) { | ||
| type.DeliveryChannels = def['@notification.deliveryChannels'].map(ch => { | ||
| if (!ch.channel) return null | ||
| const entry = { Type: resolveEnum(ch.channel).toUpperCase() } |
There was a problem hiding this comment.
Bug: resolveEnum may return a non-string, causing a TypeError on .toUpperCase()
resolveEnum returns val unchanged when it is not an object (e.g. a number), or when the object has neither = nor # (in which case the object itself is returned). In both cases calling .toUpperCase() on the result will throw TypeError: resolveEnum(...).toUpperCase is not a function at runtime.
Consider guarding with a String() coercion, or throwing a descriptive error when the resolved channel value is not a string.
| const entry = { Type: resolveEnum(ch.channel).toUpperCase() } | |
| const resolved = resolveEnum(ch.channel) | |
| if (typeof resolved !== 'string') throw new Error( | |
| `Event '${eventName}': deliveryChannel.channel resolved to a non-string value: ${JSON.stringify(resolved)}` | |
| ) | |
| const entry = { Type: resolved.toUpperCase() } |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| @@ -0,0 +1,325 @@ | |||
| const { notificationTypesFromModel } = require("../../../lib/compile") | |||
There was a problem hiding this comment.
Bug: The cds variable used in the afterEach and in the "Resolve {i18n>KEY} in subtitle field" test (line 235) refers to the top-level require('@sap/cds'), but it is never imported at the top of this file. The tests that call Object.defineProperty(cds, ...) directly (line 235, 237) will throw ReferenceError: cds is not defined because cds is only declared locally inside specific test blocks (lines 199, 215).
Should add const cds = require('@sap/cds') at the top of the file, and remove the redundant local declarations inside individual tests.
| const { notificationTypesFromModel } = require("../../../lib/compile") | |
| const cds = require('@sap/cds') | |
| const { notificationTypesFromModel } = require("../../../lib/compile") | |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| else cds.once("served", async () => { | ||
| const { validateNotificationTypes, readFile } = require("./lib/utils"); | ||
| const { createNotificationTypesMap } = require("./lib/notificationTypes"); | ||
| const production = cds.env.profiles?.includes("production"); | ||
| const { validateNotificationTypes, readFile } = require("./lib/utils") | ||
| const { createNotificationTypesMap } = require("./lib/notificationTypes") | ||
| const { notificationTypesFromModel } = require("./lib/compile") | ||
| const { path } = cds.utils | ||
| const production = cds.env.profiles?.includes("production") | ||
|
|
||
| const typesPath = cds.env.requires?.notifications?.types | ||
| const srvPath = path.join(cds.root, cds.env.folders.srv) | ||
| const model = await cds.load(srvPath) | ||
| const notificationTypes = [ | ||
| ...notificationTypesFromModel(model), | ||
| ...( typesPath ? readFile(typesPath) : [] ) | ||
| ] | ||
|
|
||
| // read notification types | ||
| const notificationTypes = readFile(cds.env.requires?.notifications?.types); | ||
| if (validateNotificationTypes(notificationTypes)) { | ||
| if (!production) { | ||
| const notificationTypesMap = createNotificationTypesMap(notificationTypes, true); | ||
| cds.notifications = { local: { types: notificationTypesMap } }; | ||
| const notificationTypesMap = createNotificationTypesMap(notificationTypes, true) | ||
| cds.notifications = { local: { types: notificationTypesMap } } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Bug: An error thrown by notificationTypesFromModel (e.g. the new 128-character validation) or by cds.load will result in an unhandled promise rejection inside the async callback — there is no try/catch. This can crash the process or silently swallow startup failures depending on the Node.js version, making the problem very hard to diagnose.
Consider wrapping the body in a try/catch and logging via cds.log('notifications').
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
1 participant
Validate that elements are below 128 characters. Prereq: #125