Deprecate getOriginalXmlWithIds() - #516
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe PR adds a runtime deprecation warning for ChangesSignedXml deprecation
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This change preserves existing getOriginalXmlWithIds() behavior while warning callers and documenting the supported signature-location replacement. No merge-blocking current-head risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/signed-xml.ts (1)
1281-1283: Consider keeping it as a method to avoid per-instance overhead.While the current implementation maintains backward compatibility, converting the method to a class field has memory implications: each
SignedXmlinstance now allocates its own deprecate-wrapped function. Additionally, as a property, it could be inadvertently reassigned.Consider this alternative that preserves the method signature:
/** * Returns the original xml with Id attributes added on relevant elements, must be called only after {@link computeSignature} * * @returns The original XML with IDs. * @deprecated This function is deprecated and will be removed in a future version. Use ComputeSignatureOptionsLocation to control where the signature will be placed in the original XML. */ - getOriginalXmlWithIds = deprecate((): string => { + getOriginalXmlWithIds(): string { + this.logDeprecation(); return this.originalXmlWithIds; - }, "`getOriginalXmlWithIds()` is deprecated and will be removed in a future version. Use ComputeSignatureOptionsLocation to control where the signature will be placed in the original XML."); + } + + private logDeprecation = deprecate(() => {}, "`getOriginalXmlWithIds()` is deprecated and will be removed in a future version. Use ComputeSignatureOptionsLocation to control where the signature will be placed in the original XML.");This approach:
- Keeps it as a method (avoiding per-instance function allocation)
- Prevents reassignment
- Maintains TypeScript method types
- Still emits the deprecation warning when called
Alternatively, if the current pattern is preferred, it's functional and the memory overhead may be acceptable depending on instance count.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)src/signed-xml.ts(1 hunks)test/signature-unit-tests.spec.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/signature-unit-tests.spec.ts (1)
example/example.js (4)
signedXml(39-39)sig(9-9)sig(22-22)doc(17-17)
🔇 Additional comments (5)
README.md (1)
285-285: LGTM! Clear deprecation notice with migration guidance.The deprecation documentation is well-written and provides users with a clear alternative approach using
ComputeSignatureOptionsLocation.test/signature-unit-tests.spec.ts (4)
112-113: LGTM! Correct migration to the new API.The test correctly migrates from the deprecated
getOriginalXmlWithIds()togetSignedXml().
175-180: LGTM! Correctly adjusted XPath query for the new API.The XPath change to target only the 'x' element's attributes is appropriate since
getSignedXml()returns the complete signed document including the Signature element, which has its own attributes. This prevents false positives in the attribute count.
539-549: LGTM! Verification ensures IDs are added correctly.The new verification block properly validates that ID attributes are added to the signed elements in the output XML, maintaining test coverage for functionality previously verified through
getOriginalXmlWithIds().
710-720: LGTM! Consistent ID verification pattern.This verification block follows the same pattern as the earlier one, ensuring comprehensive test coverage for ID attribute addition.
|
This just seems like the users are not aware how the signature location can be used within the |
|
The biggest of these libraries to I haven't tested if of course this is just off the cuff. |
b25ea5a to
063b976
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)src/signed-xml.ts(1 hunks)test/signature-unit-tests.spec.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/signed-xml.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T20:36:00.734Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) prefers to address the ref.uri mutation inside addAllReferences in a separate PR; removing the in-loop assignment is the desired fix but may be treated as a breaking change. Future guidance: avoid behavioral changes to ref.uri in the current PR.
Learnt from: shunkica
PR: node-saml/xml-crypto#506
File: src/signed-xml.ts:1159-1159
Timestamp: 2025-10-22T21:03:38.309Z
Learning: In node-saml/xml-crypto PR #506, the maintainer (shunkica) requested an issue to separate the overloaded Reference interface into distinct SigningReference and ValidationReference types. Initial hypothesis: signing-only (xpath, isEmptyUri, id, type), validation-only (uri, digestValue, validationError, signedReference), shared (transforms, digestAlgorithm, inclusiveNamespacesPrefixList). This should be proposed and designed in a follow-up, not altered in the current PR.
📚 Learning: 2025-10-22T21:50:05.441Z
Learnt from: shunkica
PR: node-saml/xml-crypto#0
File: :0-0
Timestamp: 2025-10-22T21:50:05.441Z
Learning: In src/signed-xml.ts Line 1099, createReferences mutates ref.uri = id during signing. Maintain this behavior for now; remove/refactor in a separate PR as previously requested by the maintainer.
Applied to files:
test/signature-unit-tests.spec.ts
🔇 Additional comments (4)
test/signature-unit-tests.spec.ts (4)
112-113: LGTM!The migration from
getOriginalXmlWithIds()togetSignedXml()is correct. Both contain the added ID attributes, which is what this test verifies.
175-180: Excellent XPath fix with clear documentation.The updated XPath
//*[local-name(.)='x']/@*correctly isolates the 'x' element's attributes, preventing the count from including signature attributes introduced bygetSignedXml(). The explanatory comment is helpful.
539-549: LGTM!The ID verification logic correctly migrates to use
getSignedXml(). The XPath queries properly locate ID attributes on the signed elements, and the assertions verify the expected incremental ID values.
710-720: LGTM!Consistent with the earlier ID verification block at lines 539-549. The migration correctly verifies ID attributes in the signed XML for the prefixed signature test case.
|
I ran some benchmarks with and without originalXmlWithIds. The speed difference is negligible, but the memory difference is significant.
|
|
Honestly, getting rid of foot-guns (must be called after...) is a really good idea. I'd like to expand the README example section a little to cover some of the cases I found already existing on GitHub. Also, a 5% increase in speed is nothing to sneeze at either. Did you just comment out the code that calls |
Basically yes. I added a options flag to skip assigning it it so I could run all the benchmarks at once. |
|
@shunkica , I'm ready to land this if you're good with it. |
d77d0fc to
be80e81
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #516 +/- ##
==========================================
+ Coverage 73.05% 75.46% +2.40%
==========================================
Files 9 9
Lines 902 1031 +129
Branches 239 268 +29
==========================================
+ Hits 659 778 +119
- Misses 143 145 +2
- Partials 100 108 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@cjbarth The only other thing I can think of is putting some migration guide ( eg: #516 (comment) ) in the readme. |
Adjust the deprecation mechanism for `getOriginalXmlWithIds()` to use a dedicated warning function. This change also clarifies the recommended alternative usage in the JSDoc and README, guiding users to employ the `location` option with `computeSignature()` followed by `getSignedXml()`.
|
@shunkica , I cleaned this up a bit. What do you think? |
LGTM |
2 participants
As per the discussion in issue #512 , this PR removes the internal usages of the getOriginalXmlWithIds() function and marks it deprecated.
Summary by CodeRabbit