Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: handling attributes on the body tag in the server for both rpc and document styles #814

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

natanbcpc
Copy link

@natanbcpc natanbcpc commented Jul 23, 2024

Description

When adding attributes to the body tag (e.g. <soap:Body soap:encodingStyle="http://schemas.xmlsoap.org/soap/encoding/">), the library currently returns the following error:

TypeError: Cannot read properties of undefined (reading 'output')
at Server._executeMethod (/usr/src/app/node_modules/strong-soap/src/server.js:337:44)
at Server._process (/usr/src/app/node_modules/strong-soap/src/server.js:207:14)
at IncomingMessage. (/usr/src/app/node_modules/strong-soap/src/server.js:112:18)
at IncomingMessage.emit (node:events:513:28)
at endReadableNT (node:internal/streams/readable:1359:12)
at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

Currently, this was only supported using the document style and when the attributes key was set to the default "attributes". This update adds support for rpc style as well and updates the document style to get the key from the server options.

Related issues

Checklist

Obs. I could not find a way to send an attribute on the body tag from the client or any test that covered the same scenario for the existing supported case (document style and attributesKey set to default "attributes"), so that's why no new unit tests were added.

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@natanbcpc natanbcpc force-pushed the fix/handling-attributes-in-server-body branch from 638518c to 792e5eb Compare July 23, 2024 19:30
@coveralls
Copy link

coveralls commented Jul 23, 2024

Pull Request Test Coverage Report for Build 10065189979

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.04%) to 84.651%

Totals Coverage Status
Change from base Build 10044216961: -0.04%
Covered Lines: 2894
Relevant Lines: 3248

💛 - Coveralls

@natanbcpc
Copy link
Author

@raymondfeng @achrinza from what I tried, there's no way to create a unit test for this change. Am I missing something? If not, can this be reviewed, please?

Copy link
Member

@dhmlau dhmlau left a comment

Choose a reason for hiding this comment

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

@natanbcpc, thanks for the contribution. Your changes look reasonable to me. I'd like to have another review before merging. Thanks.

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