Skip to content

Commit

Permalink
fix: suport complex objects for query params in api explorer
Browse files Browse the repository at this point in the history
BREAKING CHANGE: This fix has modified the api definitions described by the decorator
'param.query.object', to support Open-API's `url-encoded` definition for json query
parameters.

Previously, such parameters were described with `exploded: true` and
`style: deepObject`, i.e exploded encoding, which turned out to be problematic as explained and discussed in,
swagger-api/swagger-js#1385 and
OAI/OpenAPI-Specification#1706

```json
  {
    "in": "query",
    "style": "deepObject"
    "explode": "true",
    "schema": {}
  }
```

Exploded encoding worked for simple json objects as below but not for complex objects.

```
   http://localhost:3000/todos?filter[limit]=2
```

To address these issues with exploded queries, this fix switches definition of json
query params from the `exploded`, `deep-object` style to the `url-encoded` style
definition in Open-API spec.

LoopBack already supports receiving url-encoded payload for json query parameters.

For instance, to filter api results from the GET '/todo-list' endpoint in the
todo-list example with a specific relation, { "include": [ { "relation": "todo" } ] },
the following url-encoded query parameter can be used,

```
   http://localhost:3000/todos?filter=%7B%22include%22%3A%5B%7B%22relation%22%3A%22todoList%22%7D%5D%7D
```

The above was possible because the coercion behavior in LoopBack performed json
parsing for `deep object` style json query params before this fix. This fix has
modified that behavior by removing json parsing. Since the `exploded` `deep-object`
definition has been removed from the `param.query.object` decorator, this new
behaviour remains just an internal source code aspect as of now.

In effect, this fix only modifies the open api definitions generated from LoopBack
APIs. The 'style' and 'explode' fields are removed and the 'schema' field is moved
under 'content[application/json]'. This is the definition that supports url-encoding
as per Open-API spec.

```json
  {
    "in": "query"
    "content": {
      "application/json": {
        "schema": {}
      }
    }
  }
```

Certain client libraries (like swagger-ui or LoopBack's api explorer) necessiate
using Open-API's `url-encoded` style definition for json query params to support
"sending" url-encoded payload.

All consumers of LoopBack APIs may need to regenerate api definitions, if their
client libraries require them to do so for url-encoding.

Otherwise there wouldn't be any significant impact on API consumers.

To preserve compatibility with existing REST API clients, this change is backward
compatible. All exploded queries like `?filter[limit]=1` will continue to work for
json query params, despite the fact that they are described differently in the
OpenAPI spec.

Existing api clients will continue to work after an upgrade.

The signature of the 'param.query.object' decorator has not changed.

There is no code changes required in the LoopBack APIs after upgrading to this
fix. No method signatures or data structures are impacted.
  • Loading branch information
deepakrkris committed Feb 6, 2020
1 parent 3af4627 commit a4ef640
Show file tree
Hide file tree
Showing 8 changed files with 142 additions and 36 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,16 @@ describe('TodoListApplication', () => {
});
});

it('exploded filter conditions work', async () => {
const list = await givenTodoListInstance(todoListRepo);
await givenTodoInstance(todoRepo, {title: 'todo1', todoListId: list.id});
await givenTodoInstance(todoRepo, {title: 'todo2', todoListId: list.id});
await givenTodoInstance(todoRepo, {title: 'todo3', todoListId: list.id});

const response = await client.get('/todos').query('filter[limit]=2');
expect(response.body).to.have.length(2);
});

/*
============================================================================
TEST HELPERS
Expand Down
11 changes: 11 additions & 0 deletions examples/todo/src/__tests__/acceptance/todo.acceptance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,17 @@ describe('TodoApplication', () => {
.expect(200, [toJSON(todoInProgress)]);
});

it('exploded filter conditions work', async () => {
await givenTodoInstance({title: 'wake up', isComplete: true});
await givenTodoInstance({
title: 'go to sleep',
isComplete: false,
});

const response = await client.get('/todos').query('filter[limit]=2');
expect(response.body).to.have.length(2);
});

/*
============================================================================
TEST HELPERS
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,19 +221,18 @@ describe('Routing metadata for parameters', () => {
});

describe('@param.query.object', () => {
it('sets in:query style:deepObject and a default schema', () => {
it('sets in:query and content["application/json"]', () => {
class MyController {
@get('/greet')
greet(@param.query.object('filter') filter: object) {}
}
const expectedParamSpec = <ParameterObject>{
name: 'filter',
in: 'query',
style: 'deepObject',
explode: true,
schema: {
type: 'object',
additionalProperties: true,
content: {
'application/json': {
schema: {type: 'object', additionalProperties: true},
},
},
};
expectSpecToBeEqual(MyController, expectedParamSpec);
Expand All @@ -256,13 +255,15 @@ describe('Routing metadata for parameters', () => {
const expectedParamSpec: ParameterObject = {
name: 'filter',
in: 'query',
style: 'deepObject',
explode: true,
schema: {
type: 'object',
properties: {
where: {type: 'object', additionalProperties: true},
limit: {type: 'number'},
content: {
'application/json': {
schema: {
type: 'object',
properties: {
where: {type: 'object', additionalProperties: true},
limit: {type: 'number'},
},
},
},
},
};
Expand Down Expand Up @@ -300,11 +301,10 @@ describe('Routing metadata for parameters', () => {
name: 'filter',
in: 'query',
description: 'Search criteria',
style: 'deepObject',
explode: true,
schema: {
type: 'object',
additionalProperties: true,
content: {
'application/json': {
schema: {type: 'object', additionalProperties: true},
},
},
};
expectSpecToBeEqual(MyController, expectedParamSpec);
Expand Down
15 changes: 10 additions & 5 deletions packages/openapi-v3/src/decorators/parameter.decorator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,11 @@ export function param(paramSpec: ParameterObject) {
// generate schema if `paramSpec` has `schema` but without `type`
(isSchemaObject(paramSpec.schema) && !paramSpec.schema.type)
) {
// please note `resolveSchema` only adds `type` and `format` for `schema`
paramSpec.schema = resolveSchema(paramType, paramSpec.schema);
// If content explicitly mentioned do not resolve schema
if (!paramSpec.content) {
// please note `resolveSchema` only adds `type` and `format` for `schema`
paramSpec.schema = resolveSchema(paramType, paramSpec.schema);
}
}
}

Expand Down Expand Up @@ -212,9 +215,11 @@ export namespace param {
return param({
name,
in: 'query',
style: 'deepObject',
explode: true,
schema,
content: {
'application/json': {
schema,
},
},
...spec,
});
},
Expand Down
30 changes: 25 additions & 5 deletions packages/rest/src/__tests__/unit/coercion/paramObject.unit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,14 @@ import {test} from './utils';
const OPTIONAL_ANY_OBJECT: ParameterObject = {
in: 'query',
name: 'aparameter',
schema: {
type: 'object',
additionalProperties: true,
content: {
'application/json': {
schema: {
type: 'object',
additionalProperties: true,
},
},
},
style: 'deepObject',
explode: true,
};

const REQUIRED_ANY_OBJECT = {
Expand All @@ -35,6 +37,15 @@ describe('coerce object param - required', function() {
test(REQUIRED_ANY_OBJECT, {key: 'text'}, {key: 'text'});
});

context('valid string values', () => {
// simple object
test(REQUIRED_ANY_OBJECT, '{"key": "text"}', {key: 'text'});
// nested objects
test(REQUIRED_ANY_OBJECT, '{"include": [{ "relation" : "todoList" }]}', {
include: [{relation: 'todoList'}],
});
});

context('empty values trigger ERROR_BAD_REQUEST', () => {
test(
REQUIRED_ANY_OBJECT,
Expand Down Expand Up @@ -90,6 +101,15 @@ describe('coerce object param - optional', function() {
test(OPTIONAL_ANY_OBJECT, 'null', null);
});

context('valid string values', () => {
// simple object
test(OPTIONAL_ANY_OBJECT, '{"key": "text"}', {key: 'text'});
// nested objects
test(OPTIONAL_ANY_OBJECT, '{"include": [{ "relation" : "todoList" }]}', {
include: [{relation: 'todoList'}],
});
});

context('nested values are not coerced', () => {
test(OPTIONAL_ANY_OBJECT, {key: 'undefined'}, {key: 'undefined'});
test(OPTIONAL_ANY_OBJECT, {key: 'null'}, {key: 'null'});
Expand Down
36 changes: 32 additions & 4 deletions packages/rest/src/__tests__/unit/parser.unit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ describe('operationArgsParser', () => {
expect(args).to.eql(['<html><body><h1>Hello</h1></body></html>']);
});

context('in:query style:deepObject', () => {
context('in:query content:application/json', () => {
it('parses JSON-encoded string value', async () => {
const req = givenRequest({
url: '/?value={"key":"value"}',
Expand Down Expand Up @@ -346,6 +346,32 @@ describe('operationArgsParser', () => {
);
});

it('parses complex json object', async () => {
const req = givenRequest({
url: '/?filter={"include": [{"relation": "todoList"}]}',
});

const spec = givenOperationWithObjectParameter('filter');
const route = givenResolvedRoute(spec);

const args = await parseOperationArgs(req, route, requestBodyParser);

expect(args).to.eql([{include: [{relation: 'todoList'}]}]);
});

it('parses url-encoded complex json object', async () => {
const req = givenRequest({
url: '/?filter=%7B"include"%3A%5B%7B"relation"%3A"todoList"%7D%5D%7D',
});

const spec = givenOperationWithObjectParameter('filter');
const route = givenResolvedRoute(spec);

const args = await parseOperationArgs(req, route, requestBodyParser);

expect(args).to.eql([{include: [{relation: 'todoList'}]}]);
});

it('rejects array values encoded as JSON', async () => {
const req = givenRequest({
url: '/?value=[1,2]',
Expand Down Expand Up @@ -381,9 +407,11 @@ describe('operationArgsParser', () => {
{
name,
in: 'query',
style: 'deepObject',
explode: true,
schema,
content: {
'application/json': {
schema,
},
},
},
]);
}
Expand Down
13 changes: 10 additions & 3 deletions packages/rest/src/coercion/coerce-parameter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,14 @@ export function coerceParameter(
data: string | undefined | object,
spec: ParameterObject,
) {
const schema = spec.schema;
let schema = spec.schema;

// If a query parameter is a url encoded Json object, the schema is defined under content['application/json']
if (!schema && spec.in === 'query' && spec.content) {
const jsonSpec = spec.content['application/json'];
schema = jsonSpec.schema;
}

if (!schema || isReferenceObject(schema)) {
debug(
'The parameter with schema %s is not coerced since schema' +
Expand Down Expand Up @@ -172,9 +179,9 @@ function parseJsonIfNeeded(
): string | object | undefined {
if (typeof data !== 'string') return data;

if (spec.in !== 'query' || spec.style !== 'deepObject') {
if (spec.in !== 'query' || (spec.in === 'query' && !spec.content)) {
debug(
'Skipping JSON.parse, argument %s is not in:query style:deepObject',
'Skipping JSON.parse, argument %s is not a url encoded json object query parameter (since content field is missing in parameter schema)',
spec.name,
);
return data;
Expand Down
27 changes: 26 additions & 1 deletion packages/rest/src/coercion/validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,34 @@ export class Validator {
isAbsent(value: any) {
if (value === '' || value === undefined) return true;

const spec: ParameterObject = this.ctx.parameterSpec;
const schema: SchemaObject = this.ctx.parameterSpec.schema ?? {};
if (schema.type === 'object' && value === 'null') return true;
const valueIsNull = value === 'null' || value === null;

if (this.isUrlEncodedJsonParam()) {
// is this an url encoded Json object query parameter?
// check for NULL values
if (valueIsNull) return true;
} else if (spec.schema) {
// if parameter spec contains schema object, check if supplied value is NULL
if (schema.type === 'object' && valueIsNull) return true;
}

return false;
}

/**
* Return `true` if defined specification is for a url encoded Json object query parameter
*
* for url encoded Json object query parameters,
* schema is defined under content['application/json']
*/
isUrlEncodedJsonParam() {
const spec: ParameterObject = this.ctx.parameterSpec;

if (spec.in === 'query' && spec.content?.['application/json']?.schema) {
return true;
}
return false;
}
}

0 comments on commit a4ef640

Please sign in to comment.