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

serDes is not working with OpenAPI parameters #601

Closed
agavazov opened this issue May 20, 2021 · 9 comments
Closed

serDes is not working with OpenAPI parameters #601

agavazov opened this issue May 20, 2021 · 9 comments
Labels
enhancement New feature or request

Comments

@agavazov
Copy link

Describe the bug
When there is described parameter with format in parameters section they are not handled by serDes.

To Reproduce
Add format: customOne in requestBody and in parameters and define handler in serDes, only the requestBody will be handled.

Actual behavior
Only the requestBody formats are handled by serDes definitions.

Expected behavior
All inputs must be handled by serDes definitions.

Examples and context
index.js

const OpenApiValidator = require("express-openapi-validator");
const express = require("express");
const http = require("http");

const app = express();

app.use(express.json());
app.use(express.text());
app.use(express.urlencoded({ extended: false }));

app.use(
  OpenApiValidator.middleware({
      apiSpec: "./openapi.yaml",
      unknownFormats: ["customOne"],
      serDes: [
        {
          format: "customOne",
          deserialize: (s) => {
            return { customObjectValue: s };
          },
          serialize: (o) => o.toString(),
        }],
    },
  ),
);

app.post("/test/:param1", function(req, res, next) {
  res.json({
    "req.params.param1": req.params.param1,
    "typeof.req.params.param1": typeof req.params.param1, // fail
    "req.params.param2": req.query.param2,
    "typeof.req.params.param2": typeof req.query.param2, // fail
    "req.params.param3": req.body.param3,
    "typeof.req.params.param3": typeof req.body.param3, // success
  });
});

http.createServer(app).listen(3001);
console.log("http://127.0.0.1:3001/");

openapi.yaml

openapi: "3.0.0"
info:
  version: 1.0.0
  title: api-test-gateway

paths:
  /test/{param1}:
    post:
      description: Test serializations
      parameters:
        - name: param1
          in: path
          required: true
          schema:
            type: string
            format: customOne
        - name: param2
          in: query
          required: true
          schema:
            type: string
            format: customOne
      requestBody:
        content:
          application/json:
            schema:
              type: object
              required:
                - param3
              properties:
                param3:
                  type: string
                  format: customOne
      responses:
        "200":
          description: Successful response
          content:
            application/json:
              schema:
                type: object
@cdimascio cdimascio added the enhancement New feature or request label May 21, 2021
@cdimascio
Copy link
Owner

@pilerou would you be interested in investigating this feature. it's essentially an extension of the serDes work you delivered

@pilerou
Copy link
Contributor

pilerou commented May 21, 2021

Hi @agavazov and @cdimascio
Happy to see serDes feature is used.
I try to have a look in the next two weeks.

@pilerou
Copy link
Contributor

pilerou commented Jun 2, 2021

Hi @agavazov
I had a quick look.
I don't know why but if your schema referer with $refto a component, it's working (See : test/resources/serdes.yaml used by unit tests)

paths:
  /users/{id}:
    get:
      parameters:
        - name: id
          in: path
          required: true
          schema:
            $ref: "#/components/schemas/ObjectId"
....
components:
  schemas:
    ObjectId:
      type: string
      format: mongo-objectid
      pattern: '^[0-9a-fA-F]{24}$'

But If you replace $ref: "#/components/schemas/ObjectId" by the components type declaration it doesn't work.

paths:
  /users/{id}:
    get:
      parameters:
        - name: id
          in: path
          required: true
          schema:
            type: string
            format: mongo-objectid
            pattern: '^[0-9a-fA-F]{24}$'
....
components:
  schemas:
    ObjectId:
      type: string
      format: mongo-objectid
      pattern: '^[0-9a-fA-F]{24}$'

A quick fix for you could be to create a declaration in components section and refer to it in your route params in path section.
I look further in order to understand the problem.

@pilerou
Copy link
Contributor

pilerou commented Jun 2, 2021

Hi @cdimascio
I had a look. As it is initialized in schemaVisitor function of schema.preprocessor.ts,, Serdes is only used on :

  • components schema referenced using $ref
  • request body
  • response body

but not when the schema is declared directly in route parameters schema as explained before.
That's because schemaNodes is built only on components schemas, request bodies schemas and response bodies schemas... and handleSerdes mecanism is based on schemaNodes.

I think the solution would be to adapt gatherSchemaNodesFromPaths in order to manage the request query parameters as well and populate schemaNodes with the result that would be used in schemaVisitor.

I'm not very comfortable to make changes in gatherSchemaNodesFromPaths, I could create regression.

Do you see how it should be changed ?

@jazzzz
Copy link
Contributor

jazzzz commented Jun 4, 2021

@pilerou I tried your workaround, and effectively, when declaring the type in the components section, my deserializer is now called, but req.params still contains the original string.

@pilerou
Copy link
Contributor

pilerou commented Jun 7, 2021

@jazzzz It's strange. The test validates this use case.
serdes.spec.ts - line 56
The string value is well transform to an object (MongoDB's ObjectId in this use case)

Here is an example of serdes configuration for date-time :

{
  format : 'date-time',
  serialize: (d: Date) => {
    return d && d.toISOString();
  },
  deserialize: (s: string) => {
    return new Date(s);
  }
}

How is your serdes configuration ? Could you paste your configuration here and I test ?

@jazzzz
Copy link
Contributor

jazzzz commented Jun 7, 2021

I found the issue: my parameters are declared under the route, not under the method, like this:

paths:
  /users/{id}:
    parameters:
      - name: id
        in: path
        required: true
        schema:
          type: string
          format: mongo-objectid
          pattern: '^[0-9a-fA-F]{24}$'
    get:

The issue also happens for basic types, so it's not related to serDes.

@robertjustjones
Copy link
Contributor

I'm also seeing an issue with a deserialization of a query param. The function for the custom format never gets called.

@pilerou
Copy link
Contributor

pilerou commented Dec 20, 2021

I think i found the reason.
I can't explain it as serdes unit test works but i think I have the same issue.
Actually, AJV is doing 2 verifications :

  • in a first time, it's checking pattern. It adds an error in errors list vErrors
  • in a second time, it's checking custom format by calling ObjectId serdes deserialize function. As it's not in ObjectId format, it throws an exception transformed in Internal Server Error.

Instead of getting a BadRequest Error (400), we get an Internal Server Error (500).
Even if pattern control has been done, we can't see it in the answer.

Here is the AJV generated code (unflatten). As we can see, even if pattern check fails, it also do format check by deserializing ObjectId... and It fails.

if (typeof data2 === "string") {
              if (!pattern0.test(data2)) {
                var err = {
                  keyword: 'pattern',
                  dataPath: (dataPath || '') + '.params.id',
                  schemaPath: '#/properties/params/properties/id/pattern',
                  params: {pattern: '^[0-9a-fA-F]{24}$'},
                  message: 'should match pattern "^[0-9a-fA-F]{24}$"'
                };
                if (vErrors === null) vErrors = [err]; else vErrors.push(err);
                errors++;
              }
            }
            customRule2.errors = null;
            var errs__2 = errors;
            var valid2;
            customRule2.errors = null;
            valid2 = customRule2.call(self, data2, (dataPath || '') + '.params.id', data1, 'id', rootData);
            if (data1) data2 = data1['id'];
            if (!valid2) {
              if (Array.isArray(customRule2.errors)) {
                if (vErrors === null) vErrors = customRule2.errors; else vErrors = vErrors.concat(customRule2.errors);
                errors = vErrors.length;
                for (var i2 = errs__2; i2 < errors; i2++) {
                  var ruleErr2 = vErrors[i2];
                  if (ruleErr2.dataPath === undefined) ruleErr2.dataPath = (dataPath || '') + '.params.id';
                  ruleErr2.schemaPath = "#/properties/params/properties/id/x-eov-serdes";
                }
              } else {
                var err = {
                  keyword: 'x-eov-serdes',
                  dataPath: (dataPath || '') + '.params.id',
                  schemaPath: '#/properties/params/properties/id/x-eov-serdes',
                  params: {keyword: 'x-eov-serdes'},
                  message: 'should pass "x-eov-serdes" keyword validation'
                };
                if (vErrors === null) vErrors = [err]; else vErrors.push(err);
                errors++;
              }
            }
            var valid2 = errors === errs_2;

The best thing, I think is to deal with serialize and deserialize error with a try catch.
In catch block, we return false;

if (request) {
    if (options.serDesMap) {
      ajv.addKeyword('x-eov-serdes', {
        modifying: true,
        compile: (sch) => {
          if (sch) {
            return function validate(data, path, obj, propName) {
              if (typeof data === 'object') return true;
              if(!!sch.deserialize) {
                try {
                  obj[propName] = sch.deserialize(data);
                }
                catch(e) {
                  return false;
                }
              }
              return true;
            };
          }
          return () => true;
        },
      });
    }

If you are OK, I can add make it work in my fork and make a feature request.

The unique problem is the answer. We get two errors. We could only have one.

{
  "name": "Bad Request",
  "message": "request.params.id should match pattern \"^[0-9a-fA-F]{24}$\", request.params.id should pass \"x-eov-serdes\" keyword validation",
  "status": 400,
  "errors": [
    {
      "path": ".params.id",
      "message": "should match pattern \"^[0-9a-fA-F]{24}$\"",
      "errorCode": "pattern.openapi.validation"
    },
    {
      "path": ".params.id",
      "message": "should pass \"x-eov-serdes\" keyword validation",
      "errorCode": "x-eov-serdes.openapi.validation"
    }
  ]
}

pilerou added a commit to pilerou/express-openapi-validator that referenced this issue Dec 20, 2021
cdimascio pushed a commit that referenced this issue Dec 21, 2021
* Try catch serdes serialize and deserialize in order to avoid Internal Server Error and return BadRequest errors #601

* Fix incorrect serDes example #569
pilerou added a commit to pilerou/express-openapi-validator that referenced this issue Dec 24, 2021
cdimascio pushed a commit to pilerou/express-openapi-validator that referenced this issue Dec 24, 2021
pilerou added a commit to pilerou/express-openapi-validator that referenced this issue Dec 26, 2021
pilerou added a commit to pilerou/express-openapi-validator that referenced this issue Dec 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants