[ENGG-5291] SOAP Importer - #23
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:
WalkthroughAdds SOAP/WSDL import support to the API client importer. Introduces the xml2js dependency and its types, exports a new public Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🧹 Nitpick comments (9)
src/importers/api-client/soap/index.ts (1)
5-7: ParameterWsdlshould use camelCase.TypeScript convention is camelCase for parameters. Use
wsdlinstead ofWsdl.♻️ Proposed fix
-export const soapImporter = async (Wsdl: ImportFile): Promise<ApiClientImporterOutput> => { - return await soap.convert(Wsdl); +export const soapImporter = async (wsdl: ImportFile): Promise<ApiClientImporterOutput> => { + return await soap.convert(wsdl); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importers/api-client/soap/index.ts` around lines 5 - 7, The parameter name in the soapImporter function uses PascalCase; rename the parameter Wsdl to camelCase (wsdl) in the soapImporter signature and update all references inside the function (e.g., the call to soap.convert) to use wsdl; keep the parameter type ImportFile and the return type ApiClientImporterOutput unchanged so only the identifier is modified.src/importers/api-client/soap/soap.ts (7)
63-66:asArraytreats all falsy values as empty — including0,"", andfalse.Since the generic is unconstrained, this could silently discard valid falsy values. In practice, WSDL elements are objects so this is safe here, but a stricter check would be more robust.
♻️ Stricter null/undefined check
const asArray = <T>(input: T | T[] | undefined): T[] => { - if (!input) return []; + if (input === undefined || input === null) return []; return Array.isArray(input) ? input : [input]; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importers/api-client/soap/soap.ts` around lines 63 - 66, The helper asArray currently treats any falsy input as empty (using "if (!input)"), which will drop valid falsy values like 0, "" or false; change the check in asArray to only treat null or undefined as empty (e.g., use input == null or input === null || input === undefined) so valid falsy values are preserved while keeping the rest of the function intact; update the asArray<T>(...) implementation accordingly.
649-698: No tests are included for this new importer.The
convertfunction is the main public surface and handles complex XML parsing with multiple code paths (SOAP 1.1 vs 1.2, missing elements, invalid WSDL, etc.). Adding at least basic unit tests with sample WSDL content would significantly improve confidence.Would you like me to open an issue to track adding test coverage for the SOAP importer?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importers/api-client/soap/soap.ts` around lines 649 - 698, Add unit tests covering the public convert function to exercise its major code paths: successful conversion (mocking or using a representative WSDL to pass preProcessWsdl -> validateWsdlStructure -> convertWsdlToRQAPI and assert returned collection), XML parse failure (simulate preProcessWsdl throwing to assert SoapImportErrorType.XML_PARSE_ERROR), invalid WSDL structure (simulate validateWsdlStructure throwing a non-SoapError and assert SoapImportErrorType.INVALID_WSDL_STRUCTURE), and conversion error (simulate convertWsdlToRQAPI throwing a non-SoapError and assert INVALID_WSDL_STRUCTURE). Use the actual convert function and stub/mocking helpers for preProcessWsdl, validateWsdlStructure, buildWsdlContext, and convertWsdlToRQAPI (or feed real sample WSDL strings) so tests reference convert, preProcessWsdl, validateWsdlStructure, and convertWsdlToRQAPI explicitly.
552-572: Extract soapAction resolution into a named helper for readability.The inline IIFE to resolve
soapActionis complex and hard to follow. Extracting it into a small named function would improve clarity.♻️ Suggested refactor
Add a helper function:
const findSoapAction = ( binding: WsdlBinding, operationName: string, ): string | undefined => { const bindingOperations = asArray(getChildByTagName(binding, "operation")); const bindingOp = bindingOperations.find((op) => op.$?.name === operationName); if (!bindingOp) return undefined; const soapOperation = getChildByTagName(bindingOp, "operation"); const soapOperationNode = Array.isArray(soapOperation) ? soapOperation[0] : soapOperation; return soapOperationNode?.$?.soapAction; };Then replace the IIFE:
- soapAction: (() => { - const bindingOperations = asArray( - getChildByTagName(binding, "operation"), - ); - const bindingOp = bindingOperations.find( - (op) => op.$?.name === operationName, - ); - if (bindingOp) { - const soapOperation = getChildByTagName( - bindingOp, - "operation", - ); - const soapOperationNode = Array.isArray( - soapOperation, - ) - ? soapOperation[0] - : soapOperation; - return soapOperationNode?.$?.soapAction; - } - return undefined; - })(), + soapAction: findSoapAction(binding, operationName),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importers/api-client/soap/soap.ts` around lines 552 - 572, Extract the inline IIFE that computes soapAction into a small named helper (e.g., findSoapAction) to improve readability: implement findSoapAction(binding: WsdlBinding, operationName: string): string | undefined that uses asArray(getChildByTagName(binding, "operation")) to locate the binding operation whose $?.name matches operationName, retrieves the child "operation" node (handling array vs single node), and returns its $?.soapAction or undefined; then replace the IIFE used for soapAction with a call to findSoapAction(binding, operationName), keeping references to getChildByTagName, asArray, binding and operationName intact.
157-160:preProcessWsdlcan returnundefinedfor valid XML that isn't WSDL.If the parsed XML has no
definitionselement (e.g., a non-WSDL XML file),getChildByTagNamereturnsundefinedand the return typePromise<WsdlDefinitions>is misleading. ThevalidateWsdlStructureguard at line 165 does catch this, but the type contract ofpreProcessWsdldoesn't reflect thatundefinedis possible, which could mask issues if the function is reused.♻️ Suggested type signature fix
-const preProcessWsdl = async (content: string): Promise<WsdlDefinitions> => { +const preProcessWsdl = async (content: string): Promise<WsdlDefinitions | undefined> => {Then update the caller in
convert:- let definitions: WsdlDefinitions; + let definitions: WsdlDefinitions | undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importers/api-client/soap/soap.ts` around lines 157 - 160, preProcessWsdl currently promises WsdlDefinitions but can return undefined when parsed XML lacks a "definitions" element; change its signature to return Promise<WsdlDefinitions | undefined> and keep the existing implementation that uses getChildByTagName so the absence is represented in the type; then update callers (notably the convert function and any other call sites) to handle the undefined result—convert already uses validateWsdlStructure, so ensure its type checks align with the new signature and adjust any other callers to either guard for undefined or propagate it appropriately.
474-476:Date.now()inside the conversion makes output non-deterministic, hindering testability.Consider accepting
timestampas a parameter (defaulting toDate.now()) so tests can provide a fixed value. This is a minor concern but would help when adding tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importers/api-client/soap/soap.ts` around lines 474 - 476, The conversion function convertWsdlToRQAPI uses Date.now() internally making outputs non-deterministic; change its signature to accept an optional timestamp parameter (e.g., timestamp?: number) defaulting to Date.now() and replace currentTimestamp usage with that parameter so tests can pass a fixed timestamp; update any callers of convertWsdlToRQAPI to omit the argument (preserving current behavior) or pass deterministic timestamps in tests.
395-472: Significant code duplication in buildingCollectionRecordobjects.The same boilerplate structure (
id: "",collectionId: "",isExample: false,ownerId: "", etc.) is repeated increateVersionedCollection(lines 423-445, 449-471),convertWsdlToRQAPI(lines 624-646), andcreateSoapRequest(lines 348-362). Consider extracting a small factory to reduce duplication.♻️ Example factory helper
const createBaseRecord = ( name: string, description: string, timestamp: number, type: RQAPI.RecordType, ): Omit<RQAPI.ApiRecord | RQAPI.CollectionRecord, 'data'> => ({ id: "", name, description, collectionId: "", isExample: false, ownerId: "", deleted: false, createdBy: "", updatedBy: "", createdTs: timestamp, updatedTs: timestamp, type, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importers/api-client/soap/soap.ts` around lines 395 - 472, There is duplicated boilerplate when building RQAPI Collection/Api records (seen in createVersionedCollection, convertWsdlToRQAPI, and createSoapRequest); extract a small factory function (e.g., createBaseRecord or buildRecordMeta) that returns the common fields (id, collectionId, isExample, ownerId, deleted, createdBy, updatedBy, createdTs, updatedTs, type, name, description) as an object (use a return type like Omit<RQAPI.ApiRecord|RQAPI.CollectionRecord,'data'>), then use Object.assign or spread to add the data property in createVersionedCollection (portCol and top-level collection), convertWsdlToRQAPI, and createSoapRequest so each place only supplies the data payload and timestamp/name/description through the factory instead of repeating the boilerplate.
76-81:getVariableNamehas an unexplained heuristic for stripping a leadings.The regex
/^s[A-Z]/strips a leading lowercasesfollowed by an uppercase letter. This appears to be removing Hungarian notation prefixes, but there's no comment explaining when this applies or why. This could also incorrectly rename legitimate parameter names likesoapAction→oapAction.Consider adding a brief comment explaining the rationale, or removing this heuristic if it's not needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importers/api-client/soap/soap.ts` around lines 76 - 81, getVariableName currently strips a leading lowercase "s" followed by an uppercase letter via /^s[A-Z]/ with no explanation; either remove that heuristic or document it — update the getVariableName function to either (a) remove the special-case logic and always return paramName, or (b) keep it but add a clear comment above getVariableName explaining it removes a Hungarian-style "s" prefix only when followed by an uppercase letter (and why), and add a unit test for edge cases like "soapAction" and "sId" to prevent regressions.src/importers/api-client/soap/types.ts (1)
126-129: Remove unusedSoapServiceInfointerface or add a comment explaining its intended future use.The interface at lines 126-129 is defined but never imported or referenced elsewhere in the codebase. If it's reserved for future functionality, document this with a comment; otherwise, remove it to reduce dead code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importers/api-client/soap/types.ts` around lines 126 - 129, The SoapServiceInfo interface is declared but unused; either remove the SoapServiceInfo declaration (and any related unused imports like SoapPortInfo if now unused) to eliminate dead code, or add a short TODO comment above the interface explaining its intended future purpose and why it is kept (e.g., "Reserved for future SOAP service metadata: serviceName and ports") so reviewers know it's intentionally retained; update exports accordingly to reflect the removal or documented retention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/importers/api-client/soap/soap.ts`:
- Around line 528-574: The loop that builds SoapOperationInfo uses operationName
= operation.$?.name which can be undefined and later becomes
SoapOperationInfo.name and then an XML tag in generateSoapEnvelope; update the
rawOperations.forEach processing to skip any operation without a valid name
(operationName) or otherwise guard by continuing early, i.e., only push into
operations when operationName is a non-empty string, and ensure any lookup code
that uses operationName (the bindingOperations.find and soapAction resolution)
is only executed when operationName exists to avoid creating <undefined> tags in
generateSoapEnvelope.
- Around line 280-312: The createSoapRequest function currently always adds a
SOAPAction header; change it so that when port.soapVersion === "1.2" you embed
the operation.soapAction value as the action parameter in the Content-Type
header (e.g. application/soap+xml; charset=UTF-8; action="...") and do NOT add a
separate SOAPAction header, while keeping the existing SOAPAction header
behavior for SOAP 1.1; update the logic that constructs headers (and the
Content-Type string generation in generateSoapEnvelope or inline where
Content-Type is set) to quote the action value and only attach it for SOAP 1.2
when operation.soapAction is present.
---
Nitpick comments:
In `@src/importers/api-client/soap/index.ts`:
- Around line 5-7: The parameter name in the soapImporter function uses
PascalCase; rename the parameter Wsdl to camelCase (wsdl) in the soapImporter
signature and update all references inside the function (e.g., the call to
soap.convert) to use wsdl; keep the parameter type ImportFile and the return
type ApiClientImporterOutput unchanged so only the identifier is modified.
In `@src/importers/api-client/soap/soap.ts`:
- Around line 63-66: The helper asArray currently treats any falsy input as
empty (using "if (!input)"), which will drop valid falsy values like 0, "" or
false; change the check in asArray to only treat null or undefined as empty
(e.g., use input == null or input === null || input === undefined) so valid
falsy values are preserved while keeping the rest of the function intact; update
the asArray<T>(...) implementation accordingly.
- Around line 649-698: Add unit tests covering the public convert function to
exercise its major code paths: successful conversion (mocking or using a
representative WSDL to pass preProcessWsdl -> validateWsdlStructure ->
convertWsdlToRQAPI and assert returned collection), XML parse failure (simulate
preProcessWsdl throwing to assert SoapImportErrorType.XML_PARSE_ERROR), invalid
WSDL structure (simulate validateWsdlStructure throwing a non-SoapError and
assert SoapImportErrorType.INVALID_WSDL_STRUCTURE), and conversion error
(simulate convertWsdlToRQAPI throwing a non-SoapError and assert
INVALID_WSDL_STRUCTURE). Use the actual convert function and stub/mocking
helpers for preProcessWsdl, validateWsdlStructure, buildWsdlContext, and
convertWsdlToRQAPI (or feed real sample WSDL strings) so tests reference
convert, preProcessWsdl, validateWsdlStructure, and convertWsdlToRQAPI
explicitly.
- Around line 552-572: Extract the inline IIFE that computes soapAction into a
small named helper (e.g., findSoapAction) to improve readability: implement
findSoapAction(binding: WsdlBinding, operationName: string): string | undefined
that uses asArray(getChildByTagName(binding, "operation")) to locate the binding
operation whose $?.name matches operationName, retrieves the child "operation"
node (handling array vs single node), and returns its $?.soapAction or
undefined; then replace the IIFE used for soapAction with a call to
findSoapAction(binding, operationName), keeping references to getChildByTagName,
asArray, binding and operationName intact.
- Around line 157-160: preProcessWsdl currently promises WsdlDefinitions but can
return undefined when parsed XML lacks a "definitions" element; change its
signature to return Promise<WsdlDefinitions | undefined> and keep the existing
implementation that uses getChildByTagName so the absence is represented in the
type; then update callers (notably the convert function and any other call
sites) to handle the undefined result—convert already uses
validateWsdlStructure, so ensure its type checks align with the new signature
and adjust any other callers to either guard for undefined or propagate it
appropriately.
- Around line 474-476: The conversion function convertWsdlToRQAPI uses
Date.now() internally making outputs non-deterministic; change its signature to
accept an optional timestamp parameter (e.g., timestamp?: number) defaulting to
Date.now() and replace currentTimestamp usage with that parameter so tests can
pass a fixed timestamp; update any callers of convertWsdlToRQAPI to omit the
argument (preserving current behavior) or pass deterministic timestamps in
tests.
- Around line 395-472: There is duplicated boilerplate when building RQAPI
Collection/Api records (seen in createVersionedCollection, convertWsdlToRQAPI,
and createSoapRequest); extract a small factory function (e.g., createBaseRecord
or buildRecordMeta) that returns the common fields (id, collectionId, isExample,
ownerId, deleted, createdBy, updatedBy, createdTs, updatedTs, type, name,
description) as an object (use a return type like
Omit<RQAPI.ApiRecord|RQAPI.CollectionRecord,'data'>), then use Object.assign or
spread to add the data property in createVersionedCollection (portCol and
top-level collection), convertWsdlToRQAPI, and createSoapRequest so each place
only supplies the data payload and timestamp/name/description through the
factory instead of repeating the boilerplate.
- Around line 76-81: getVariableName currently strips a leading lowercase "s"
followed by an uppercase letter via /^s[A-Z]/ with no explanation; either remove
that heuristic or document it — update the getVariableName function to either
(a) remove the special-case logic and always return paramName, or (b) keep it
but add a clear comment above getVariableName explaining it removes a
Hungarian-style "s" prefix only when followed by an uppercase letter (and why),
and add a unit test for edge cases like "soapAction" and "sId" to prevent
regressions.
In `@src/importers/api-client/soap/types.ts`:
- Around line 126-129: The SoapServiceInfo interface is declared but unused;
either remove the SoapServiceInfo declaration (and any related unused imports
like SoapPortInfo if now unused) to eliminate dead code, or add a short TODO
comment above the interface explaining its intended future purpose and why it is
kept (e.g., "Reserved for future SOAP service metadata: serviceName and ports")
so reviewers know it's intentionally retained; update exports accordingly to
reflect the removal or documented retention.
| const createSoapRequest = ( | ||
| operation: SoapOperationInfo, | ||
| port: SoapPortInfo, | ||
| context: WsdlContext, | ||
| timestamp: number, | ||
| ): { record: RQAPI.ApiRecord; variables: string[] } => { | ||
| const params = getOperationParams(operation, context); | ||
| const soapEnvelope = generateSoapEnvelope(port.soapVersion, params); | ||
|
|
||
| const headers: KeyValuePair[] = [ | ||
| { | ||
| id: 1, | ||
| key: "Content-Type", | ||
| value: | ||
| port.soapVersion === "1.2" | ||
| ? "application/soap+xml; charset=UTF-8" | ||
| : "text/xml; charset=UTF-8", | ||
| isEnabled: true, | ||
| description: "SOAP Content-Type header", | ||
| dataType: KeyValueDataType.STRING, | ||
| }, | ||
| ]; | ||
|
|
||
| if (operation.soapAction) { | ||
| headers.push({ | ||
| id: 2, | ||
| key: "SOAPAction", | ||
| value: operation.soapAction, | ||
| isEnabled: true, | ||
| description: "SOAP Action header", | ||
| dataType: KeyValueDataType.STRING, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
SOAP 1.2 specification SOAPAction header deprecation Content-Type action parameter
💡 Result:
In SOAP 1.2 over HTTP, the SOAP 1.1 SOAPAction HTTP header is removed; instead, the “action” information (if used) is carried in the action parameter of the application/soap+xml Content-Type header. [1]
What to send in SOAP 1.2
A SOAP 1.2 request typically uses:
Content-Type: application/soap+xml; charset=utf-8- Optionally:
; action="absolute-URI"(quotes are commonly used in practice)
Example:
POST /service HTTP/1.1
Content-Type: application/soap+xml; charset=utf-8; action="http://example.com/MyOperation"The action parameter is optional and is intended as a hint for routing/dispatch; receivers should not require it to function. If present, its value must be a non-empty absolute URI-reference. [2]
Relationship to the SOAP 1.2 spec text
SOAP 1.2 defines a “SOAP Action Feature” whose value, when used with MIME/HTTP, is represented specifically as the action parameter in the media type (Content-Type), and is mapped back by the receiver from that parameter. [3]
Backward-compat / interoperability guidance
WS-I Basic Profile 2.0 guidance commonly cited by toolkits says:
- Sender should not include
SOAPAction. - Receiver must not rely on
SOAPActionfor correct processing. [4]
Sources: W3C SOAP 1.2 Primer diff (SOAPAction removed; moved to Content-Type action) [1]; RFC 3902 media type registration (action semantics and optionality) [2]; W3C SOAP 1.2 Part 2 action feature mapping to Content-Type action [3]; WS-I BP 2.0 interoperability rules as quoted by Apache CXF issue (R2760/R2761) [4].
SOAP 1.2 must use the action parameter in Content-Type header instead of the deprecated SOAPAction header.
The SOAP 1.2 specification removes the SOAPAction HTTP header entirely. For SOAP 1.2 requests over HTTP, the action information is conveyed via the action parameter in the Content-Type header (e.g., application/soap+xml; charset=UTF-8; action="..."). The action parameter is optional but, when present, must be an absolute URI-reference. Currently, the code incorrectly adds a SOAPAction header for both SOAP 1.1 and 1.2, which violates the SOAP 1.2 specification.
🐛 Proposed fix
const headers: KeyValuePair[] = [
{
id: 1,
key: "Content-Type",
value:
port.soapVersion === "1.2"
- ? "application/soap+xml; charset=UTF-8"
+ ? `application/soap+xml; charset=UTF-8${operation.soapAction ? `; action="${operation.soapAction}"` : ""}`
: "text/xml; charset=UTF-8",
isEnabled: true,
description: "SOAP Content-Type header",
dataType: KeyValueDataType.STRING,
},
];
- if (operation.soapAction) {
+ if (operation.soapAction && port.soapVersion === "1.1") {
headers.push({
id: 2,
key: "SOAPAction",
value: operation.soapAction,
isEnabled: true,
description: "SOAP Action header",
dataType: KeyValueDataType.STRING,
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/importers/api-client/soap/soap.ts` around lines 280 - 312, The
createSoapRequest function currently always adds a SOAPAction header; change it
so that when port.soapVersion === "1.2" you embed the operation.soapAction value
as the action parameter in the Content-Type header (e.g. application/soap+xml;
charset=UTF-8; action="...") and do NOT add a separate SOAPAction header, while
keeping the existing SOAPAction header behavior for SOAP 1.1; update the logic
that constructs headers (and the Content-Type string generation in
generateSoapEnvelope or inline where Content-Type is set) to quote the action
value and only attach it for SOAP 1.2 when operation.soapAction is present.
There was a problem hiding this comment.
https://www.w3.org/TR/2000/NOTE-SOAP-20000508/#_Toc478383528
Currently the header is SOAPAction as detailed here. Action works for some but it is better to follow the SOAPAction header defined here as although it is optional and deprecated it is needed for most legacy systems
There was a problem hiding this comment.
@shubhranshu-dash, thank you for the clarification! You're absolutely right that maintaining compatibility with legacy systems is a valid and practical consideration. Many SOAP implementations, especially in enterprise environments, still expect the SOAPAction header even when using SOAP 1.2.
Since you're intentionally keeping the SOAPAction header for both SOAP 1.1 and 1.2 to support legacy systems, I'd suggest adding a brief code comment to document this deliberate design choice. This will help future maintainers understand why the implementation deviates from the strict SOAP 1.2 specification:
// Note: SOAPAction header is included for both SOAP 1.1 and 1.2 for compatibility
// with legacy systems, even though SOAP 1.2 spec recommends using the 'action'
// parameter in Content-Type instead.
if (operation.soapAction) {
headers.push({
id: 2,
key: "SOAPAction",
value: operation.soapAction,
isEnabled: true,
description: "SOAP Action header",
dataType: KeyValueDataType.STRING,
});
}This approach prioritizes real-world interoperability over strict spec compliance, which is often the right trade-off for importers.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/importers/api-client/soap/soap.ts (4)
562-582: RecomputesbindingOperationsarray on every loop iteration.The IIFE inside
rawOperations.forEachcallsasArray(getChildByTagName(binding, "operation"))on each pass. Hoisting it outside the loop makes the intent clearer and avoids the repeated traversal.♻️ Proposed refactor
+ const bindingOperations = asArray( + getChildByTagName<WsdlBindingOperation>(binding, "operation"), + ); + rawOperations.forEach((operation) => { const operationName = operation.$?.name; if (!operationName) return; ... operations.push({ name: operationName, messageName: inputMsg.replace(/^.*:/, ""), inputMessageObject, - soapAction: (() => { - const bindingOperations = asArray( - getChildByTagName<WsdlBindingOperation>(binding, "operation"), - ); - const bindingOp = bindingOperations.find( - (op) => op.$?.name === operationName, - ); + soapAction: (() => { + const bindingOp = bindingOperations.find( + (op) => op.$?.name === operationName, + ); if (bindingOp) { ... } return undefined; })(), }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importers/api-client/soap/soap.ts` around lines 562 - 582, The IIFE inside rawOperations.forEach recomputes bindingOperations by calling asArray(getChildByTagName(binding, "operation")) on every iteration; hoist that call out of the loop (compute bindingOperations once before the forEach) and then inside the loop simply find the bindingOp (using bindingOperations.find(...)) and extract soapAction from soapOperationNode as currently done; update references to bindingOperations, bindingOp, soapOperation, and operationName so the logic is identical but avoids repeated getChildByTagName/asArray calls.
562-582:bindingOperationsarray is rebuilt on every loop iteration.The IIFE calls
asArray(getChildByTagName(binding, "operation"))once per operation in therawOperations.forEach, making the total work O(n × m). Hoisting the array out of the loop is a trivial fix.♻️ Proposed refactor
+ const bindingOperations = asArray( + getChildByTagName<WsdlBindingOperation>(binding, "operation"), + ); + rawOperations.forEach((operation) => { ... operations.push({ ... - soapAction: (() => { - const bindingOperations = asArray( - getChildByTagName<WsdlBindingOperation>(binding, "operation"), - ); - const bindingOp = bindingOperations.find( + soapAction: (() => { + const bindingOp = bindingOperations.find( (op) => op.$?.name === operationName, ); ... })(), }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importers/api-client/soap/soap.ts` around lines 562 - 582, The code rebuilds bindingOperations each iteration by calling asArray(getChildByTagName(binding, "operation")) inside the IIFE used to compute soapAction; hoist that call out of the loop (compute const bindingOperations = asArray(getChildByTagName(binding, "operation")) once before rawOperations.forEach) and then replace the IIFE body with a simple lookup using bindingOperations.find(op => op.$?.name === operationName) and the existing soapOperation extraction logic to return soapAction; this avoids repeated work and keeps the existing getChildByTagName, bindingOp, soapOperation, and operationName logic intact.
139-157:Array.isArrayternaries inconsistent with theasArrayhelper used everywhere else.♻️ Proposed refactor
- const ct = Array.isArray(complexType) - ? complexType[0] - : complexType; + const ct = asArray(complexType)[0]; const sequence = getChildByTagName<any>(ct, "sequence"); if (sequence) { - const seq = Array.isArray(sequence) - ? sequence[0] - : sequence; + const seq = asArray(sequence)[0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importers/api-client/soap/soap.ts` around lines 139 - 157, The code inconsistently uses Array.isArray ternaries for complexType and sequence instead of the project's asArray helper; replace those ternaries by wrapping complexType and sequence in asArray (using getChildByTagName as currently used) and then selecting the first element (e.g., const ct = asArray(complexType)[0]; const seq = asArray(sequence)[0];) before calling getChildByTagName and asArray for elements so the logic in getChildByTagName, complexType, sequence, childElements, and the children mapping is uniform with the rest of the codebase.
139-157: Inconsistent use ofArray.isArrayinstead of theasArrayhelper.The
complexTypeandsequenceunwrapping uses manualArray.isArrayternaries; the rest of the file usesasArray. Replacing these keeps the code uniform.♻️ Proposed refactor
- const ct = Array.isArray(complexType) - ? complexType[0] - : complexType; + const ct = asArray(complexType)[0]; const sequence = getChildByTagName<any>(ct, "sequence"); if (sequence) { - const seq = Array.isArray(sequence) - ? sequence[0] - : sequence; + const seq = asArray(sequence)[0]; const childElements = asArray(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importers/api-client/soap/soap.ts` around lines 139 - 157, Replace the manual Array.isArray ternaries with the asArray helper to normalize arrays consistently: use asArray(complexType)[0] to obtain ct instead of Array.isArray(complexType) ? complexType[0] : complexType, and after calling getChildByTagName(ct, "sequence") use asArray(sequence)[0] for seq instead of the Array.isArray(sequence) ternary; keep the subsequent getChildByTagName(seq, "element") and asArray(...) mapping logic so behavior is unchanged while matching the file's asArray usage pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/importers/api-client/soap/soap.ts`:
- Around line 663-671: The try/catch around preProcessWsdl currently always
wraps any thrown error as SoapImportErrorType.XML_PARSE_ERROR; update the catch
so that if the caught error is already a SoapError (i.e., has a .type) you
rethrow it unchanged, otherwise wrap it with
createSoapError(SoapImportErrorType.XML_PARSE_ERROR, "Failed to parse WSDL XML
content", error). Locate the block with preProcessWsdl and modify the catch to
check (error as SoapError).type and rethrow when present before creating a new
XML_PARSE_ERROR.
- Around line 663-671: The catch around await preProcessWsdl(file.content)
currently always wraps any thrown error as XML_PARSE_ERROR; change it to first
detect if the caught error is already a SoapError (e.g., check (error as
SoapError).type or error.type) and rethrow it unchanged when present (so
INVALID_WSDL_STRUCTURE and other specific SoapImportErrorType values are
preserved), otherwise wrap only non-Soap errors with
createSoapError(SoapImportErrorType.XML_PARSE_ERROR, "Failed to parse WSDL XML
content", error); keep references to preProcessWsdl, SoapImportErrorType, and
createSoapError to locate the spot.
- Around line 347-348: The regex replacement on requestName (computed from
operation.name || operation.messageName) can produce an empty string (e.g.,
"SoapRequest"), leaving the ApiRecord.name empty; update the logic around
requestName to detect an empty result after
requestName.replace(/Soap(12)?Request$/i, "") and fall back to a sensible value
(for example the original operation.name or operation.messageName, or a default
like "Request") before creating the ApiRecord so ApiRecord.name is never empty;
adjust code paths that construct the ApiRecord to use this validated
requestName.
- Around line 347-348: The regex replacement on requestName can produce an empty
string (e.g., "SoapRequest"), so after computing requestName (from
operation.name || operation.messageName) and performing
requestName.replace(/Soap(12)?Request$/i, ""), check if the resulting
requestName is empty or whitespace and if so restore a sensible fallback (e.g.,
the original unmodified name or operation.messageName) before creating the
ApiRecord; update the code around the requestName variable to preserve the
original (e.g., originalName) and use that as the fallback when requestName ===
"".
---
Duplicate comments:
In `@src/importers/api-client/soap/soap.ts`:
- Around line 297-329: The createSoapRequest function currently always adds a
SOAPAction header when operation.soapAction is present; update it so that for
port.soapVersion === "1.2" you instead append the action parameter to the
existing Content-Type header value (e.g., include ; action="...") and do NOT
push a separate SOAPAction header, while for SOAP 1.1 keep the current behavior
of adding the SOAPAction header; modify logic around the headers array and the
Content-Type value construction in createSoapRequest to conditionally format
Content-Type and skip adding the SOAPAction header when soapVersion is "1.2",
using the symbols createSoapRequest, port.soapVersion, operation.soapAction, and
headers to locate the change.
- Around line 297-329: The createSoapRequest function currently adds a
SOAPAction header even for SOAP 1.2; change the logic so that when
port.soapVersion === "1.2" and operation.soapAction is present you DO NOT push a
separate SOAPAction header but instead include the action as the action
parameter on the Content-Type header (e.g., application/soap+xml; charset=UTF-8;
action="..."); for SOAP 1.1 keep the existing SOAPAction header behavior. Locate
createSoapRequest, the initial Content-Type header in headers and the
conditional that pushes SOAPAction, and either build the Content-Type value with
the action for SOAP 1.2 (properly quoted) or modify headers[0].value accordingly
while skipping the headers.push for SOAPAction when soapVersion === "1.2".
---
Nitpick comments:
In `@src/importers/api-client/soap/soap.ts`:
- Around line 562-582: The IIFE inside rawOperations.forEach recomputes
bindingOperations by calling asArray(getChildByTagName(binding, "operation")) on
every iteration; hoist that call out of the loop (compute bindingOperations once
before the forEach) and then inside the loop simply find the bindingOp (using
bindingOperations.find(...)) and extract soapAction from soapOperationNode as
currently done; update references to bindingOperations, bindingOp,
soapOperation, and operationName so the logic is identical but avoids repeated
getChildByTagName/asArray calls.
- Around line 562-582: The code rebuilds bindingOperations each iteration by
calling asArray(getChildByTagName(binding, "operation")) inside the IIFE used to
compute soapAction; hoist that call out of the loop (compute const
bindingOperations = asArray(getChildByTagName(binding, "operation")) once before
rawOperations.forEach) and then replace the IIFE body with a simple lookup using
bindingOperations.find(op => op.$?.name === operationName) and the existing
soapOperation extraction logic to return soapAction; this avoids repeated work
and keeps the existing getChildByTagName, bindingOp, soapOperation, and
operationName logic intact.
- Around line 139-157: The code inconsistently uses Array.isArray ternaries for
complexType and sequence instead of the project's asArray helper; replace those
ternaries by wrapping complexType and sequence in asArray (using
getChildByTagName as currently used) and then selecting the first element (e.g.,
const ct = asArray(complexType)[0]; const seq = asArray(sequence)[0];) before
calling getChildByTagName and asArray for elements so the logic in
getChildByTagName, complexType, sequence, childElements, and the children
mapping is uniform with the rest of the codebase.
- Around line 139-157: Replace the manual Array.isArray ternaries with the
asArray helper to normalize arrays consistently: use asArray(complexType)[0] to
obtain ct instead of Array.isArray(complexType) ? complexType[0] : complexType,
and after calling getChildByTagName(ct, "sequence") use asArray(sequence)[0] for
seq instead of the Array.isArray(sequence) ternary; keep the subsequent
getChildByTagName(seq, "element") and asArray(...) mapping logic so behavior is
unchanged while matching the file's asArray usage pattern.
| let requestName = operation.name || operation.messageName; | ||
| requestName = requestName.replace(/Soap(12)?Request$/i, ""); |
There was a problem hiding this comment.
requestName can become an empty string after regex substitution.
If operation.name is "SoapRequest" (or matches the entire regex), the replacement reduces it to "", leaving the ApiRecord with an empty name.
🐛 Proposed fix
let requestName = operation.name || operation.messageName;
requestName = requestName.replace(/Soap(12)?Request$/i, "");
+ if (!requestName) requestName = operation.name;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/importers/api-client/soap/soap.ts` around lines 347 - 348, The regex
replacement on requestName (computed from operation.name ||
operation.messageName) can produce an empty string (e.g., "SoapRequest"),
leaving the ApiRecord.name empty; update the logic around requestName to detect
an empty result after requestName.replace(/Soap(12)?Request$/i, "") and fall
back to a sensible value (for example the original operation.name or
operation.messageName, or a default like "Request") before creating the
ApiRecord so ApiRecord.name is never empty; adjust code paths that construct the
ApiRecord to use this validated requestName.
requestName can become an empty string after the regex substitution.
If operation.name is exactly "SoapRequest" or "SOAP12REQUEST" (or any casing variant the regex matches in full), the replacement reduces it to "", producing an ApiRecord with a blank name.
🐛 Proposed fix
let requestName = operation.name || operation.messageName;
requestName = requestName.replace(/Soap(12)?Request$/i, "");
+ if (!requestName) requestName = operation.name;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/importers/api-client/soap/soap.ts` around lines 347 - 348, The regex
replacement on requestName can produce an empty string (e.g., "SoapRequest"), so
after computing requestName (from operation.name || operation.messageName) and
performing requestName.replace(/Soap(12)?Request$/i, ""), check if the resulting
requestName is empty or whitespace and if so restore a sensible fallback (e.g.,
the original unmodified name or operation.messageName) before creating the
ApiRecord; update the code around the requestName variable to preserve the
original (e.g., originalName) and use that as the fallback when requestName ===
"".
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/importers/api-client/soap/soap.ts (1)
504-506: Guard against blank request names after suffix stripping.This is still vulnerable to producing an empty
ApiRecord.namewhen the full string is stripped by the regex.🐛 Proposed fix
- let requestName = operation.name || operation.messageName; - requestName = requestName.replace(/(Soap(12)?Request|Soap(11|12)|Soap(In|Out)|Port|Binding)$/i, ""); + let requestName = operation.name || operation.messageName || "Request"; + const originalRequestName = requestName; + requestName = requestName + .replace(/(Soap(12)?Request|Soap(11|12)|Soap(In|Out)|Port|Binding)$/i, "") + .trim(); + if (!requestName) requestName = originalRequestName || "Request";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importers/api-client/soap/soap.ts` around lines 504 - 506, The regex strip on requestName (computed from operation.name || operation.messageName in the soap importer) can yield an empty string; after calling requestName.replace(...) ensure you guard and fall back to a non-empty identifier (e.g., the original unstripped operation.name/operation.messageName or a safe default like `${operation.name || operation.messageName}-Request`) before assigning to ApiRecord.name; update the logic around the requestName variable in the soap.ts function so if the stripped value is empty you assign the fallback to guarantee ApiRecord.name is never blank.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/importers/api-client/soap/soap.ts`:
- Around line 401-406: The RPC encoded envelope generation uses a
soapenv:encodingStyle attribute even though only the soap prefix is declared,
producing invalid XML; in the block that builds bodyXml (where style === "rpc"
and encStyle is set when use === "encoded"), either change the injected prefix
to the declared one (soap:encodingStyle) or ensure the soapenv prefix is
declared on the envelope (add
xmlns:soapenv="http://schemas.xmlsoap.org/soap/envelope/") so the encStyle
attribute references a valid namespace; update the code that constructs encStyle
(and/or the envelope namespace declarations) accordingly so rpc/encoded
envelopes emit a valid namespaced encodingStyle attribute.
---
Duplicate comments:
In `@src/importers/api-client/soap/soap.ts`:
- Around line 504-506: The regex strip on requestName (computed from
operation.name || operation.messageName in the soap importer) can yield an empty
string; after calling requestName.replace(...) ensure you guard and fall back to
a non-empty identifier (e.g., the original unstripped
operation.name/operation.messageName or a safe default like `${operation.name ||
operation.messageName}-Request`) before assigning to ApiRecord.name; update the
logic around the requestName variable in the soap.ts function so if the stripped
value is empty you assign the fallback to guarantee ApiRecord.name is never
blank.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/importers/api-client/soap/index.tssrc/importers/api-client/soap/soap.tssrc/importers/api-client/soap/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/importers/api-client/soap/index.ts
| let bodyXml = ""; | ||
| if (style === "rpc") { | ||
| const encStyle = use === "encoded" ? ` soapenv:encodingStyle="http://schemas.xmlsoap.org/soap/encoding/"` : ""; | ||
| const partsXml = bodyParts.map(p => paramToXml(p, use, "", 3)).join("\n"); | ||
| bodyXml = ` <tns:${operationName}${encStyle}>\n${partsXml}\n </tns:${operationName}>`; | ||
| } else { |
There was a problem hiding this comment.
Fix invalid XML namespace prefix in RPC encoded envelope generation.
Line 403 injects soapenv:encodingStyle, but no soapenv prefix is declared (only soap). This can generate malformed envelopes for rpc/encoded.
🐛 Proposed fix
- if (style === "rpc") {
- const encStyle = use === "encoded" ? ` soapenv:encodingStyle="http://schemas.xmlsoap.org/soap/encoding/"` : "";
+ if (style === "rpc") {
+ const encodingStyleUri =
+ soapVersion === "1.2"
+ ? "http://www.w3.org/2003/05/soap-encoding"
+ : "http://schemas.xmlsoap.org/soap/encoding/";
+ const encStyle =
+ use === "encoded" ? ` soap:encodingStyle="${encodingStyleUri}"` : "";
const partsXml = bodyParts.map(p => paramToXml(p, use, "", 3)).join("\n");
bodyXml = ` <tns:${operationName}${encStyle}>\n${partsXml}\n </tns:${operationName}>`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/importers/api-client/soap/soap.ts` around lines 401 - 406, The RPC
encoded envelope generation uses a soapenv:encodingStyle attribute even though
only the soap prefix is declared, producing invalid XML; in the block that
builds bodyXml (where style === "rpc" and encStyle is set when use ===
"encoded"), either change the injected prefix to the declared one
(soap:encodingStyle) or ensure the soapenv prefix is declared on the envelope
(add xmlns:soapenv="http://schemas.xmlsoap.org/soap/envelope/") so the encStyle
attribute references a valid namespace; update the code that constructs encStyle
(and/or the envelope namespace declarations) accordingly so rpc/encoded
envelopes emit a valid namespaced encodingStyle attribute.
rohanmathur91
left a comment
rohanmathur91
left a comment
There was a problem hiding this comment.
Code Review: SOAP Importer
Overview
This PR adds a WSDL parser (892 lines in soap.ts, 124 lines in types.ts) that converts WSDL 1.1 definitions into Requestly API collections. The implementation handles SOAP 1.1/1.2 detection, document/rpc styles, literal/encoded use, complex type inheritance with cycle detection, and SOAP header extraction. Solid feature scope.
Bugs
1. isWsdl() gives wrong error for WSDL 2.0 files
soap.ts:125-131
isWsdl() only checks for the WSDL 1.1 namespace (http://schemas.xmlsoap.org/wsdl/). A WSDL 2.0 file (namespace http://www.w3.org/ns/wsdl) fails this check and gets the generic "Invalid WSDL file format" error at line 867. The better "WSDL 2.0 is not supported" error inside preProcessWsdl (line 206) is never reached.
Fix: add the WSDL 2.0 namespace to the check so it passes through to preProcessWsdl:
const isWsdl = (content: string): boolean => {
if (!content) return false;
const hasWsdl11 = content.includes("http://schemas.xmlsoap.org/wsdl/");
const hasWsdl20 = content.includes("http://www.w3.org/ns/wsdl");
const hasDefinitions = content.includes("definitions");
const hasDescription = content.includes("description");
return (hasWsdl11 && hasDefinitions) || (hasWsdl20 && hasDescription);
};2. Unqualified type references silently ignored
soap.ts:289-298
extractParamsFromNode only follows type references that contain : (namespace prefix). For an element like <xsd:element name="foo" type="MyType"/> (no prefix on type), the type resolution is skipped entirely. The element falls through to the complexType check, which looks up context.typeMap.has(node.$?.name) — checking the element name ("foo") not the type name ("MyType").
Fix: remove the typeAttr.includes(":") guard — always try to resolve:
const typeAttr = node.$?.type;
if (typeAttr) {
const cleanType = typeAttr.replace(/^.*:/, "");
if (visited.has(cleanType)) return [];
visited.add(cleanType);
const typeInfo = context.typeMap.get(cleanType);
if (typeInfo) return extractParamsFromNode(typeInfo.node, context, visited);
return [];
}3. requestName can become empty string
soap.ts:536-537
If operation.name is "SoapRequest" or "Binding", the regex strips the entire name to "". Add a fallback:
requestName = requestName.replace(/(Soap(12)?Request|Soap(11|12)|Soap(In|Out)|Port|Binding)$/i, "");
if (!requestName) requestName = operation.name || operation.messageName || "Unnamed Operation";Code Quality
4. console.warn in library code
soap.ts:716
console.warn(`Port ${portName} has no SOAP address location`);Library code shouldn't write to the consumer's console. Either silently skip (already does with return) or collect warnings in the output.
5. bindingOperations re-queried inside loop
soap.ts:756-758
asArray(getChildByTagName<WsdlBindingOperation>(binding, "operation")) is called for every iteration of rawOperations.forEach(). The binding doesn't change — hoist it above the loop.
6. as any type bypass
soap.ts:784-785
style: style as any,
use: use as any,style and use come from XML string attributes, which could be anything. Rather than casting away type safety, validate before assignment:
const validStyles = ["document", "rpc"] as const;
const validUses = ["literal", "encoded"] as const;
style: validStyles.includes(style as any) ? (style as "document" | "rpc") : "document",
use: validUses.includes(use as any) ? (use as "literal" | "encoded") : "literal",7. Array.isArray inconsistency
Four places use Array.isArray(x) ? x[0] : x instead of asArray(x)[0]:
soap.ts:233(detectSoapVersion)soap.ts:306(extractParamsFromNode)soap.ts:710-712(soapAddress)soap.ts:726(globalSoapBinding)
Use asArray consistently since it also handles undefined.
8. No tests
892 lines of parsing logic with recursive type resolution, cycle detection, RPC/document handling, and two SOAP versions — zero tests. This is the single biggest risk in the PR. At minimum:
- One document/literal WSDL
- One RPC/encoded WSDL
- One WSDL with complex type inheritance
- One WSDL 2.0 file (should error gracefully)
- One malformed XML (should error gracefully)
Minor / Nits
9. types.ts:1 — Stale file-path comment // src/importers/api-client/soap/types.ts at top of file. Remove.
10. soap.ts:120-123 — getVariableName is just identity + fallback. Consider inlining it.
11. index.ts — Missing trailing newline.
Limitations (fine for v1, worth tracking)
- No
<wsdl:import>/<xsd:import>/<xsd:include>support — Multi-file WSDLs (common in enterprise) will produce incomplete envelopes. Consider detecting these and warning the user. Date.now()not injectable — Makes deterministic testing harder.
Summary
| Category | Count |
|---|---|
| Bugs | 3 |
| Code quality | 5 |
| Nits | 3 |
| Tracked limitations | 2 |
The three bugs (#1 wrong error for WSDL 2.0, #2 unqualified type resolution, #3 empty request name) and the lack of tests (#8) are the items I'd want addressed before merge. Everything else is incremental improvement.
3 participants
Summary by CodeRabbit
New Features
Chores