Deprecate getOriginalXmlWithIds() #516
Conversation
WalkthroughDeprecates SignedXml.getOriginalXmlWithIds (replacing the method with a deprecate-wrapped property), updates README deprecation guidance, adjusts exported declarations, and migrates tests to use getSignedXml with narrowed XPath assertions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Potential review focus:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-10-22T21:50:05.454ZApplied to files:
📚 Learning: 2025-10-22T21:03:38.354ZApplied to files:
📚 Learning: 2025-10-25T14:41:13.728ZApplied to files:
🧬 Code graph analysis (1)test/signature-unit-tests.spec.ts (1)
🔇 Additional comments (5)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
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. |
shunkica
force-pushed
the
deprecate-originalxmlwithids
branch
from
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. |
shunkica
force-pushed
the
deprecate-originalxmlwithids
branch
from
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. |
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