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

Changes in Response Class contructor for status range and null body status check #1706

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion @types/index.test-d.ts
Expand Up @@ -37,7 +37,6 @@ async function run() {
// Post
try {
const request = new Request('http://byjka.com/buka');
new Request(new URL('http://byjka.com/buka'));
expectType<string>(request.url);
expectType<Headers>(request.headers);

Expand Down
3 changes: 2 additions & 1 deletion package.json
Expand Up @@ -17,7 +17,8 @@
"test": "mocha",
"coverage": "c8 report --reporter=text-lcov | coveralls",
"test-types": "tsd",
"lint": "xo"
"lint": "xo",
"fix:lint": "xo --fix"
},
"repository": {
"type": "git",
Expand Down
31 changes: 27 additions & 4 deletions src/response.js
Expand Up @@ -10,6 +10,22 @@ import {isRedirect} from './utils/is-redirect.js';

const INTERNALS = Symbol('Response internals');

/**
* Checks the provided 'status' property to the Response constructor
* @param {*} status
* @throws RangeError if the status variable is not in the acceptable range of [200, 599]
* @returns The provided status value if it is valid
*/
function validateStatusValue(status) {
const statusValue = Number.parseInt(status, 10);

if (Number.isNaN(statusValue) || statusValue < 200 || statusValue > 599) {
throw new RangeError(`Failed to construct 'Response': The status provided (${status}) is outside the range [200, 599].`);
}

return statusValue;
};

/**
* Response class
*
Expand All @@ -23,12 +39,18 @@ export default class Response extends Body {
constructor(body = null, options = {}) {
super(body, options);

// eslint-disable-next-line no-eq-null, eqeqeq, no-negated-condition
const status = options.status != null ? options.status : 200;
Copy link

Choose a reason for hiding this comment

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

this check with coersion has covered the case options.status !== undefined, so you can simply do
const status = options.status != null ? validateStatusValue(options.status) : 200;
but I'm not fussed, happy with either. :)

Copy link
Author

Choose a reason for hiding this comment

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

@jazelly in this code

const status = options.status != null ? validateStatusValue(options.status) : 200;

if the status is undefined( when the client/application hasnt prvovided the value), the default value 200 should be assumed, but the function validateStatusValue will be throwing an error. That's why added the check for undefined as well.

Copy link

Choose a reason for hiding this comment

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

Yes, but notice it's a !=. When options.status is undefined, options.status != null is falsy, and will set a default 200 to the status.

// if status is not provided or is null, let's use default 200 value
// otherwise, lets check if the value is in correct range
const status = (typeof options.status === 'undefined' || options.status === null) ? 200 :
validateStatusValue(options.status);

const headers = new Headers(options.headers);

if (body !== null && !headers.has('Content-Type')) {
if (status === 204 && typeof body !== 'undefined' && body !== null) {
Copy link

Choose a reason for hiding this comment

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

I think we also need 205 and 304, as per the standard

throw new TypeError("Failed to construct 'Response': Response with null body status cannot have body")
}

if (body !== null && !headers.has('Content-Type')) {
const contentType = extractContentType(body, this);
if (contentType) {
headers.append('Content-Type', contentType);
Expand Down Expand Up @@ -119,7 +141,8 @@ export default class Response extends Body {
}

static error() {
const response = new Response(null, {status: 0, statusText: ''});
// default error status code = 400
const response = new Response(null, {status: 400, statusText: ''});

This comment was marked as outdated.

response[INTERNALS].type = 'error';
return response;
}
Expand Down
22 changes: 21 additions & 1 deletion test/response.js
Expand Up @@ -248,11 +248,31 @@ describe('Response', () => {
expect(await res.text()).to.equal('a');
});

it('should throw a RangeError when options.status in the constructor is value outside of the range [200, 599]', async () => {
const inputStatus = 0
try {
new Response('a', { status: inputStatus })
} catch (err) {
expect(err).to.be.instanceOf(RangeError)
expect(err.message).to.equal(`Failed to construct 'Response': The status provided (${inputStatus}) is outside the range [200, 599].`)
}
})

it('should throw a TypeError when when a non-null body is provided with a null body status', async () => {
asingh04 marked this conversation as resolved.
Show resolved Hide resolved
const inputStatus = 204
try {
new Response('a', { status: inputStatus })
} catch (err) {
expect(err).to.be.instanceOf(TypeError)
expect(err.message).to.equal("Failed to construct 'Response': Response with null body status cannot have body")
}
})

it('should support error() static method', () => {
const res = Response.error();
expect(res).to.be.an.instanceof(Response);
expect(res.type).to.equal('error');
expect(res.status).to.equal(0);
expect(res.status).to.equal(400);
expect(res.statusText).to.equal('');
});

Expand Down