fix: cloud sdk - #451
Conversation
![hyperspace-pr-bot[bot]](https://cdn.github.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The switch from direct assignment to Object.defineProperty is likely the intended fix (e.g., to patch a read-only exported property), but both calls are missing writable: true and configurable: true, which silently freezes those properties and can break re-patching or tests.
PR Bot Information
Version: 1.26.14
- Correlation ID:
6bf1b950-76e8-11f1-877b-299e7adc465b - LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.opened - File Content Strategy: Full file content
| } | ||
| }) | ||
| cloudSDK.executeHttpRequestWithOrigin = wrap(_executeWithOrigin, { | ||
| Object.defineProperty(cloudSDK, 'executeHttpRequest', { value: _executeHttpRequest }) |
There was a problem hiding this comment.
Bug: Object.defineProperty without writable: true or configurable: true defaults both to false, making the property non-writable and non-configurable. This is a stricter result than the original simple assignment, and will silently fail (or throw in strict mode) if any downstream code tries to reassign or redefine the property (e.g. in tests or repeated patching).
| Object.defineProperty(cloudSDK, 'executeHttpRequest', { value: _executeHttpRequest }) | |
| Object.defineProperty(cloudSDK, 'executeHttpRequest', { value: _executeHttpRequest, writable: true, configurable: true }) |
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
| }) | ||
| } | ||
| }) | ||
| Object.defineProperty(cloudSDK, 'executeHttpRequestWithOrigin', { value: _executeHttpRequestWithOrigin }) |
There was a problem hiding this comment.
Bug: Same issue — writable and configurable default to false, making executeHttpRequestWithOrigin frozen on the module export.
| Object.defineProperty(cloudSDK, 'executeHttpRequestWithOrigin', { value: _executeHttpRequestWithOrigin }) | |
| Object.defineProperty(cloudSDK, 'executeHttpRequestWithOrigin', { value: _executeHttpRequestWithOrigin, writable: true, configurable: true }) |
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
Uses Object.defineProperty to patch executeHttpRequest and executeHttpRequestWithOrigin on the @sap-cloud-sdk/http-client module exports, which may be non-writable. Adds writable: true, configurable: true so the properties remain re-patchable (e.g. by tests or re-instrumentation).
SummaryThe following content is AI-generated and provides a summary of the pull request: Fix Cloud SDK HTTP Request Tracing via
|
There was a problem hiding this comment.
The writable: true, configurable: true flags are already present in the current diff — my previous comments have been addressed. The code looks correct. The test file change simply sets process.env.cds_remote_native__fetch = 'true' at the top level, which is a sensible setup for OpenTelemetry HTTP instrumentation to intercept outbound calls during testing.
The PR is in good shape. Both Object.defineProperty calls already include writable: true and configurable: true as my earlier comments suggested, and the test file change correctly forces native fetch before any module is loaded. No further issues to raise.
PR Bot Information
Version: 1.29.14
- LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.ready_for_review - Experiment Variant:
control - File Content Strategy: Full file content
- Correlation ID:
b42aa6c0-8d20-11f1-8315-2d3e2e9f6717
1 participant
What
@sap-cloud-sdk/http-clientmay exportexecuteHttpRequestandexecuteHttpRequestWithOriginas non-writable / non-configurable properties. Direct assignment (cloudSDK.executeHttpRequest = wrap(...)) silently fails in that case.Fix: use
Object.definePropertyto patch both functions, withwritable: true, configurable: trueso the properties remain re-patchable (tests, re-instrumentation, future SDK versions).Changes
lib/tracing/cloud_sdk.js— swap direct assignment forObject.defineProperty(..., { value, writable: true, configurable: true }); also reformats multi-linetrace(...)calls for consistency.Test plan
npx jest test/tracing-span-names.test.js test/tracing-attributes.test.js test/tracing.test.js— all passnpm run lint— clean