fix(http): Handle undefined serialized JSON Query param

[acote-coveo]

Description

When a query parameter has content-type like JSON, an undefined serialized value could result into a Javascript Runtime error.

If we look at the code, we can see that if the parameter has a content property, we have the following handling:

swagger-js/src/execute/oas3/parameter-builders.js

Lines 30 to 35 in e0194da

	if (parameter.content) {
	const effectiveMediaType = Object.keys(parameter.content)[0];
	req.query[parameter.name] = serialize(value, effectiveMediaType);
	return;
	}

Then, if look into the serialize function, we see that for JSON, it will either serialize the value or return it (if it's a string:

swagger-js/src/execute/oas3/content-serializer.js

Lines 6 to 16 in e0194da

	export default function serialize(value, mediaType) {
	if (mediaType.includes('application/json')) {
	if (typeof value === 'string') {
	// Assume the user has a JSON string
	return value;
	}
	return JSON.stringify(value);
	}
	return value.toString();
	}

The JSON.stringify() was returning an undefined value if it was provided with a undefined value. Thus, it was breaking a bunch of assumptions in the code to expect the value to be an object or a string.

Motivation and Context

Whenever we would define a query parameter of content type "application/json", if no value or example was provided for this value, it will result in a runtime error. Consequently, we were able to use the Try it out feature of Swagger UI on the endpoints having this particularity.

It potentially fixes swagger-api/swagger-ui#8374. I haven't tested but it the same error.

How Has This Been Tested?

I added unit test to see if the error still happen and I tested the change locally by linking my local instance of SwaggerUI with my local SwaggerClient repo.

I made sure there was no JS error when trying an endpoint and made sure the parameter were properly passed.

Screenshots (if appropriate):

Types of changes

• No code changes (changes to documentation, CI, metadata, etc)
• Dependency changes (any modification to dependencies in package.json)
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[char0n]

Hi @acote-coveo,

Thanks for contributing I'll look in this PR ASAP.

[char0n]

This will be processed when [email protected] (https://github.com/swagger-api/swagger-js/releases/tag/v3.19.0-beta.5) is out of beta, probably during next week. The timing needs to be sequential here.

[char0n]

All right, we're in stable release channel again, I'll process this PR during this week.

[char0n]

This else block is AFAICT never going to be hit. If we look at serialize function:

export default function serialize(value, mediaType) {
  if (mediaType.includes('application/json')) {
    if (typeof value === 'string') {
      // Assume the user has a JSON string
      return value;
    }
    return JSON.stringify(value);
  }

  return value.toString();
}

If always returns undefined or string. It returns undefined for following cases:

serialize(undefined, 'application/json'); // => undefined
serialize(Symbol.for('test'), 'application/json'); // => undefined
...etc...

We've now established that the serializedValue variable can be either string or undefined, this serves as a proof that else is never going to be hit.

What do you think, if we would simplify the code into following statement?

  if (parameter.content) {
    const effectiveMediaType = Object.keys(parameter.content)[0];

    value = serialize(value, effectiveMediaType);
    if (value) {
      req.query[parameter.name] = value;
    }
    return;
  }

[acote-coveo]

We've now established that the serializedValue variable can be either string or undefined, this serves as a proof that else is never going to be hit.

The else if is comparing the value to undefined. So the simplification you're suggesting is not entirely equivalent.

Take this example:

const value = Symbol.for('test') // Symbol(test)
serialize(value, 'application/json'); // undefined

// Invalidation of your proof
value !== undefined // true

Note that I haven't look at all the possible combination of input/output we could have for the serialize functional. So I took the safest path and kept the same else that I saw elsewhere in the code.

[char0n]

You're right, we're comparing serializedValue and value. So taking my falsy "elaboration" back.

[swagger-bot]

🎉 This PR is included in version 3.19.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀