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

Array[number] query params is NOT correctly parsed. #5793

Closed
1 task
shamisonn opened this issue Jun 20, 2020 · 8 comments
Closed
1 task

Array[number] query params is NOT correctly parsed. #5793

shamisonn opened this issue Jun 20, 2020 · 8 comments

Comments

@shamisonn
Copy link

shamisonn commented Jun 20, 2020

Steps to reproduce

I create HelloController, it can get request parameters by array query strings

export class HelloController {
  constructor() {}

  @get("/hello_array_parse", {
    responses: {
      "200": HELLO_RESPONSE,
    },
  })
  async helloArrayParse(
    @param.array("numberArray", "query", {
      description: "number array",
      items: {
        type: "number",
        pattern: "[0-9]{4}",
      },
    }) numberArray: number[],
    @param.array("stringArray", "query", {
      description: "string array",
      items: {
        type: "string",
        pattern: "[a-zA-Z]{4}",
      },
    }) stringArray: string[],
  ): Promise<object> {
    return {
      numberArray: numberArray,
      stringArray: stringArray,
    };
  }

  @get("/hello_comma_array", {
    responses: {
      "200": HELLO_RESPONSE,
    },
  })
  async helloCommaArray(
    // ref. https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#style-values
    @param({
      name: "numberArray",
      in: "query",
      description: "number array",
      schema: {
        type: "array",
        items: {
          type: "number",
          pattern: "[0-9]{4}",
        },
      },
      style: "form",
      explode: false,
    }) numberArray: number[],
    @param({
      name: "stringArray",
      in: "query",
      description: "string array",
      schema: {
        type: "array",
        items: {
          type: "string",
          pattern: "[a-zA-Z]{4}",
        },
      },
      style: "form",
      explode: false,
    }) stringArray: string[],
  ): Promise<object> {
    return {
      numberArray: numberArray,
      stringArray: stringArray,
    };
  }
}

In detail: https://github.com/shamisonn/reproduce-lb4-bug-oas-array

Current Behavior

case1: request query by array params

request for /hello_array_parse

http://[::1]:3000/hello_array_parse?numberArray=12345&numberArray=123&stringArray=abcde&stringArray=abc

response

{
  "numberArray": [
    "12345",
    "123"
  ],
  "stringArray": [
    "abcde",
    "abc"
  ]
}

case2: request query by array params which delimited by comma

request for /hello_comma_parse

http://[::1]:3000/hello_comma_array?numberArray=12345,678&stringArray=abcde,fgh

response

{
  "numberArray": "12345,678",
  "stringArray": "abcde,fgh"
}

Expected Behavior

both of case1 and case2 are same expected like below, if pattern option is nothing.
I expected numberArray is parsed type of array[number].

{
  "numberArray": [
    12345,
    123
  ],
  "stringArray": [
    "abcde",
    "abc"
  ]
}

if pattern is there, expected error.

Link to reproduction sandbox

In guideline, Modify the selected example project. but I wrote by orginal.
if you ok, check my original example.

Additional information

$ node -e 'console.log(process.platform, process.arch, process.versions.node)'
darwin x64 14.3.0
$ npm ls --prod --depth 0 | grep loopback
├── @loopback/[email protected]
├── @loopback/[email protected]
├── @loopback/[email protected]
├── @loopback/[email protected]
├── @loopback/[email protected]
├── @loopback/[email protected]
├── @loopback/[email protected]

I don't have one myself, but I was hoping we could implement it here: https://github.com/strongloop/loopback-next/blob/master/packages/rest/src/coercion/coerce-parameter.ts#L58-L81
like this:

...
case 'array':
  return coerceArray(data, spec);

Related Issues

I can't find. if duplicate , close this.

Thank you for reading!

See Reporting Issues for more tips on writing good issues

Acceptance Criteria

@shamisonn shamisonn added the bug label Jun 20, 2020
@achrinza
Copy link
Member

achrinza commented Jun 20, 2020

Hi @shamisonn, thanks for the detailed issue! There seems to be 3 issues here:

1. Param array coercion does not work

@param.array() ignores the TypeScript type and OAS3 type: 'number'. This is unexpected behaviour.

I've managed to replicate the issue from my end with the sandbox.

2. Accidental nested array

@param.array() will automatically transpose the object into items. So there's no need to use items unless if you're creating a nested array.

For example, the generated OAS3 spec from the sandbox is:

...
"parameters": [
  {
    "name": "numberArray",
    "in": "query",
    "schema": {
      "type": "array",
      "items": {
        "description": "number array",
        "items": {
          "type": "number",
          "pattern": "[0-9]{4}"
        }
      }
    }
  },
  {
    "name": "stringArray",
    "in": "query",
    "schema": {
      "type": "array",
      "items": {
        "description": "string array",
        "items": {
          "type": "string",
          "pattern": "[a-zA-Z]{4}"
        }
      }
    }
  }
],
...

Notice the nested items.items. Frankly, I'm surprised that this considered valid OAS3 spec, and that the API Explorer accepts it. It may be an area we might also want to investigate.

To resolve, we can remove the items object:

    @param.array('numberArray', 'query', {
      description: 'number array',
-     items: {
        type: 'number',
        pattern: '[0-9]{4}',
-     },
    })
    numberArray: number[],
    @param.array('stringArray', 'query', {
      description: 'string array',
-     items: {
        type: 'string',
        pattern: '[a-zA-Z]{4}',
-     },
    })
    stringArray: string[],

This would result in the following OAS3 spec:

...
"parameters": [
  {
    "name": "numberArray",
    "in": "query",
    "schema": {
      "type": "array",
      "items": {
        "description": "number array",
        "type": "number",
        "pattern": "[0-9]{4}"
      }
    }
  },
  {
    "name": "stringArray",
    "in": "query",
    "schema": {
      "type": "array",
      "items": {
        "description": "string array",
        "type": "string",
        "pattern": "[a-zA-Z]{4}"
      }
    }
  }
],
...

Notice that there's no more nested items.items key.

This, however, does not resolve the first issue.

This also reveals a third issue:

3. Certain properties are placed in items

description (along with some other properties) should not be placed inside of items. I'm also surprised that the API explorer accepts this as valid OAS3 spec.

This ties in with #4645,

@shamisonn
Copy link
Author

shamisonn commented Jun 20, 2020

@achrinza

thank you for early reply!

@param.array() will automatically transpose the object into items.
...
description (along with some other properties) should not be placed inside of items

oh, I'll fix the sandbox. thank you for details.

(edited at 2020/06/20) fixed sandbox => shamisonn/reproduce-lb4-bug-oas-array@d7bd2bd

@jannyHou
Copy link
Contributor

jannyHou commented Jun 22, 2020

Thank you @achrinza and @shamisonn Good catch. This is loosely related to the story I am fixing in #4992

Let me summarize the bug here. I tried with Explorer inputs, not working either.

Description

Parsing array value from argument decorated by @param.array() has bug, it doesn't respect the item type specified for each item.

Given controller method

 async helloArrayParse(
    @param.array("numberArray", "query", {
      type: "number",
      pattern: "[0-9]{4}",
    }) numberArray: number[],
    @param.array("stringArray", "query", {
      type: "string",
      pattern: "[a-zA-Z]{4}",
    }) stringArray: string[],
  ): Promise<object> {
    return {
      numberArray: numberArray,
      stringArray: stringArray,
    };
  }

Current Behaviour

Provide some number inputs for the first array param

Screen Shot 2020-06-22 at 1 37 20 PM

The returned data is not coerced to number, but a string instead.

Screen Shot 2020-06-22 at 1 37 26 PM

Expected Behaviour

{
  "numberArray": [
    // DIFFERENCE: The values should be numbers instead of strings.
    12345,
    123
  ],
  "stringArray": [
    "12345",
    "123"
  ]
}

@dhmlau dhmlau added the 2020Q4 label Aug 10, 2020
@nflaig
Copy link
Member

nflaig commented Oct 5, 2020

There is another issue I noticed that if you only pass one value e.g. ?numberArray=123 then it throws the error with the code INVALID_PARAMETER_VALUE and in details with the message should be array, however ?numberArray=123&numberArray=12345 works just fine.

I also noticed that ?numberArray[]=123 works but thats different from how it used to work and also is not how the api explorer sends the values.

Seems like there is a general issue with the @param.array with parameter location query

@dhmlau dhmlau added this to the Nov 2020 milestone Oct 18, 2020
@Swanseatom
Copy link

There is another issue I noticed that if you only pass one value e.g. ?numberArray=123 then it throws the error with the code INVALID_PARAMETER_VALUE and in details with the message should be array, however ?numberArray=123&numberArray=12345 works just fine.

I also noticed that ?numberArray[]=123 works but thats different from how it used to work and also is not how the api explorer sends the values.

Seems like there is a general issue with the @param.array with parameter location query

Also notice the same issue as you mention.

@mrmodise
Copy link
Contributor

mrmodise commented Nov 5, 2020

There is another issue I noticed that if you only pass one value e.g. ?numberArray=123 then it throws the error with the code INVALID_PARAMETER_VALUE and in details with the message should be array, however ?numberArray=123&numberArray=12345 works just fine.

I also noticed that ?numberArray[]=123 works but thats different from how it used to work and also is not how the api explorer sends the values.

Seems like there is a general issue with the @param.array with parameter location query

This one is resolved. I tried to reproduce the issue above and am not getting it. Will have a look again

@stale
Copy link

stale bot commented Jul 14, 2021

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

@stale stale bot added the stale label Jul 14, 2021
@stale
Copy link

stale bot commented Aug 13, 2021

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

@stale stale bot closed this as completed Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants