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.

Sign up for GitHub

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

Jump to bottom

Gavel.js should print an invalid media type #88

Closed
honzajavorek opened this issue Mar 2, 2017 · 8 comments · May be fixed by jshttp/media-typer#10
Closed

Gavel.js should print an invalid media type #88

honzajavorek opened this issue Mar 2, 2017 · 8 comments · May be fixed by jshttp/media-typer#10
Assignees
@artem-zakharchenko
Labels
easy to fix improvement in progress

Comments

@honzajavorek
Copy link
Contributor

Given API Blueprint like this:

+ Response 200 (image)

Gavel throws an error that the media type is invalid. It would be nice if the error message contains the actual value. Instead of:

TypeError: invalid media type

there would be:

TypeError: invalid media type 'image'
@honzajavorek honzajavorek mentioned this issue Mar 2, 2017
Closed
@artem-zakharchenko
Copy link
Contributor

I assume this error message comes from mediaTyper (or similar) library responsible for parsing the given media type. It would be nice for that library to include the raw input in the error message. Perhaps, we can issue a pull request to them.

@artem-zakharchenko artem-zakharchenko mentioned this issue May 20, 2019
Open
2 tasks
@artem-zakharchenko artem-zakharchenko self-assigned this May 22, 2019
@artem-zakharchenko artem-zakharchenko mentioned this issue May 22, 2019
Open
@artem-zakharchenko
Copy link
Contributor

I've issued a pull request into media-typer package to improve the returned errors. It's up to the package's maintainers to cooperate on it. Hoping it would get merged.

@artem-zakharchenko
Copy link
Contributor

Since there has been no visible progress on this issue in almost a month, I've pinged the media-typer library's maintainer to update us on the status. I hope we will get his reply soon.

@artem-zakharchenko
Copy link
Contributor

The media-typer maintainer did not respond to the pull request, so I suppose we still should cover this under gavel?

@honzajavorek
Copy link
Contributor Author

Yes, we could.

@artem-zakharchenko
Copy link
Contributor

Let's analyze where would the users of Gavel experience such error:

  1. mediaTyper.parse(). During media type parsing it's impossible to propagate the mentioned error to the consumer, since we suppress any exceptions during body media type parsing:

gavel.js/lib/units/validateBody.js

Lines 41 to 48 in a6b65a4

function parseContentType(contentType) {
try {
const { type } = contentTypeUtils.parse(`${contentType}`);
return mediaTyper.parse(type);
} catch (error) {
return null;
}
}

  1. mediaTyper.format(). Formatting is used for error messages when actual body type cannot be validated against the actual body type. In that case such error message must propagate to the end consumer of Gavel.

gavel.js/lib/units/validateBody.js

Lines 147 to 152 in a6b65a4

if (!validator) {
const error = `Can't validate actual media type '${mediaTyper.format(
realType
)}' against the expected media type '${mediaTyper.format(expectedType)}'.`;
return [error, null, null];
}

Let's analyze each of these two calls of mediaTyper.format() in more detail.

  • format(expectedType). It can throw here if "Content-Type" header is a malformed string that cannot be parsed. We should add a test for this.
  • format(realType). The "Content-Type" header from the actual HTTP message. It's very unlikely for it to be malformed (although possible). I would consider this a corner case.

@artem-zakharchenko
Copy link
Contributor

After further investigation, it appears impossible for expectedType to be a malformed media type. Elaborating below.

When determining the body type we are using this function:

gavel.js/lib/units/validateBody.js

Lines 68 to 88 in a6b65a4

function getBodyType(body, contentType, httpMessageOrigin) {
const hasJsonContentType = isJsonContentType(contentType);
try {
parseJson(body);
const bodyMediaType = parseContentType(
hasJsonContentType ? contentType : 'application/json'
);
return [null, bodyMediaType];
} catch (parsingError) {
const fallbackMediaType = mediaTyper.parse('text/plain');
const error = hasJsonContentType
? `Can't validate: ${httpMessageOrigin} body 'Content-Type' header is '${contentType}' \
but body is not a parseable JSON:
${parsingError.message}`
: null;
return [error, fallbackMediaType];
}
}

In case we have a "Content-Type" header, and it's malformed, this function will go through the catch statement, and the "text/plain" media type will be used instead. So, in case of malformed "Content-Type" header Gavel attempts to validate the actual body as plain text.

This being said, the input to mediaTyper.format() cannot be malformed. The output from getBodyType is always a valid media type.

That being said, at the moment I don't see a scenario when Gavel would output the invalid media type where such would be included.

@artem-zakharchenko
Copy link
Contributor

Closing this as it's not possible to reach the scenario when mediaTyper error would be visible to the end consumer of the library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@artem-zakharchenko artem-zakharchenko

Labels
easy to fix improvement in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Includes invalid values in thrown errors artem-zakharchenko/media-typer
2 participants
@honzajavorek @artem-zakharchenko