Skip to content

fix: make status in getInstanceByBusinessKey optional for imported process services#54

Merged
SirSimon04 merged 6 commits intomainfrom
fixImportGetInstanceParameter
Mar 24, 2026
Merged

fix: make status in getInstanceByBusinessKey optional for imported process services#54
SirSimon04 merged 6 commits intomainfrom
fixImportGetInstanceParameter

Conversation

@tilwbr
Copy link
Contributor

@tilwbr tilwbr commented Mar 23, 2026

  • on process import, the function getInstanceByBusinessKey made the parameter status not optional, although it should
  • this fix changes the behavior on process import so that the status is optional

@tilwbr tilwbr force-pushed the fixImportGetInstanceParameter branch from 8de110d to 79d90c8 Compare March 23, 2026 10:15
Copy link
Contributor

@hyperspace-insights hyperspace-insights bot left a comment

Choose a reason for hiding this comment

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

The PR correctly removes the intermediate ProcessInstanceStatus type alias and makes status directly an optional many String parameter on getInstancesByBusinessKey. However, two categories of issues were found:

  1. Non-null assertions in validation-utils.ts: Making CsnElement.type optional in the interface was the right modelling change, but the six resulting ! assertions throughout convertTreeToElementTypes and getAllElementTypes only silence TypeScript — they don't protect against undefined at runtime. Any element backed solely by items (an inline array with no type field) will produce undefined in ElementType.type, breaking the string contract silently. Each site needs a real guard or fallback.

  2. Spurious length: 256 on an array parameter in processImport.ts: length is a scalar constraint and has no effect on an items-typed parameter; it belongs inside the items spec if a length constraint on individual strings is intended, or should simply be removed.

PR Bot Information

Version: 1.19.1 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback

  • LLM: anthropic--claude-4.6-sonnet
  • Event Trigger: pull_request.opened
  • Correlation ID: 0a19d010-26a1-11f1-9245-0d9a27830d9f

Copy link
Contributor

@hyperspace-insights hyperspace-insights bot left a comment

Choose a reason for hiding this comment

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

The PR makes two focused changes: (1) removes the intermediate ProcessInstanceStatus type alias and replaces the status parameter with an inline many String in the generated CDS, and (2) makes CsnElement.type optional in the type definition, adding ?? '' fallbacks throughout validation-utils.ts. The core logic for the status parameter fix is correct, but the ?? '' fallback for an undefined type is a minor concern — it silently converts a missing type to an empty string rather than surfacing it clearly. All other changes (test fixtures, checksums, and the new getAllInstancesByBusinessKey test helper) look correct and consistent.

PR Bot Information

Version: 1.19.1 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback

  • Event Trigger: issue_comment.created
  • Correlation ID: ac9920b0-26a2-11f1-821e-7c0ffdda71a7
  • LLM: anthropic--claude-4.6-sonnet

@hyperspace-insights hyperspace-insights bot deleted a comment from tilwbr Mar 23, 2026
@tilwbr tilwbr marked this pull request as ready for review March 23, 2026 11:51
@tilwbr tilwbr requested a review from a team as a code owner March 23, 2026 11:51
@hyperspace-insights
Copy link
Contributor

Summary

The following content is AI-generated and provides a summary of the pull request:


Fix: Make status Parameter Optional in getInstancesByBusinessKey for Imported Process Services

Bug Fix

🐛 During process import, the getInstancesByBusinessKey function incorrectly treated the status parameter as non-optional. This fix removes the intermediate ProcessInstanceStatus type and replaces it with an inline many String type, making the status parameter properly optional.

Changes

  • lib/processImport.ts: Removed the generation of the ProcessInstanceStatus type definition. Updated the getInstancesByBusinessKey action to use { items: { type: csn.CdsBuiltinType.String } } inline instead of referencing the now-removed statusType.
  • lib/build/validation-utils.ts: Made the type field optional (type?) in the ElementType interface to support elements defined via items without an explicit type.
  • lib/types/csn-extensions.ts: Made the type field optional (type?) in the CsnElement interface to allow array-typed elements (using items) without requiring a type reference.
  • tests/bookshop/srv/programmatic-service.ts: Removed the status parameter from getInstancesByBusinessKey calls to align with the optional behavior.
  • tests/bookshop/package.json: Removed a stale ShipmentHandlerService process service entry.
  • Multiple generated .cds fixture files (bookshop and hybrid): Removed the ProcessInstanceStatus type and updated the status parameter in getInstancesByBusinessKey from status : ProcessInstanceStatus to status : many String. Checksums updated accordingly.

  • 🔄 Regenerate and Update Summary
  • ✏️ Insert as PR Description (deletes this comment)
  • 🗑️ Delete comment
PR Bot Information

Version: 1.19.1 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback

  • Correlation ID: a8ab2730-26ae-11f1-9d16-280815978ddc
  • Output Template: Default Template
  • Event Trigger: pull_request.ready_for_review
  • LLM: anthropic--claude-4.6-sonnet
  • Summary Prompt: Default Prompt

💌 Have ideas or want to contribute? Create an issue and share your thoughts with us!
📑 Check out the documentation for more information.
📬 Subscribe to the Hyperspace PR Bot DL to get the latest announcements and pilot features!

Made with ❤️ by Hyperspace.

Copy link
Contributor

@hyperspace-insights hyperspace-insights bot left a comment

Choose a reason for hiding this comment

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

The suggestion I posted was incorrect (using a non-existent isArray property). Let me reconsider and wrap up.

The core change in this PR is straightforward: replace the named ProcessInstanceStatus type alias (many String) with an inline items: { type: csn.CdsBuiltinType.String } directly on the status parameter. Looking at the generated .cds files, the output status : many String is valid CDS syntax, and the CDS compiler does support inline many on function parameters. The type? changes in CsnElement and ElementType are necessary to support elements that use items without a type property (array parameters). These are correct and consistent with how CDS models arrays.

The PR is clean and focused. The existing reviewer comments flag test changes in tests/bookshop/srv/programmatic-service.ts and tests/hybrid/programmaticApproach.test.ts — the author should address those concerns about whether the test changes are necessary and whether hybrid tests are the right place for them. No additional substantive issues were found beyond what's already been raised.

PR Bot Information

Version: 1.19.1 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback

  • Event Trigger: pull_request.ready_for_review
  • LLM: anthropic--claude-4.6-sonnet
  • Correlation ID: a8ab2730-26ae-11f1-9d16-280815978ddc

@SirSimon04 SirSimon04 merged commit f531143 into main Mar 24, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants