Add serDes setting : serialize and deserialize

[pilerou]

serDes setting allows to serialize response objects and deserialize request parameters or fields. It could resolve : #353 #465 #288 #246
Unit tests validate Date and MongoDb ObjectID.
Developers have choice to :

• only serialize response contents
• also deserialize request strings to custom objects

Frequent SerDes are defined in base.serdes.ts (date and date-time).

Documentation updated with this setting

[pilerou]

@cdimascio I added serDes feature.
In this version, user needs to add serDes setting to middleware if he wants to serialize Date to date or date-time.
If you prefer, we could add a default serDes configuration at middleware init when no serDes is defined. This default serDes would add a serDes definition as if developer configuration were :

serDes: [{
  OpenApiValidator.baseSerDes.dateTimeSerializeOnly,
  OpenApiValidator.baseSerDes.dateSerializeOnly,
}],

[pilerou]

Sorry. Some tests fail.
I'm looking why.

[pilerou]

@cdimascio It's good ! I fixed problems and, after reading you comments on #493, I added a default SerDes in order serialize date and date-time by default on response when no serDes is defined.

All tests passed.

I have a last question on defaultSerDes behaviour. It could be used :

• only when no "serDes" setting is configured (as it is commit in this PR)
• as a base : serDes middleware setting would be added to default (date and datetime) serDes

If you prefer the second solution, I can do it quickly.

[cdimascio]

@pilerou Awesome! This is great.

review nit:

• can you add a test where e.g. a Date object should be provided, but an incorrect object was provided. Want to make sure that still results in a 500

i like the idea of making this the default behavior for serdes. would making this the default break existing users?

finally,I'm sure many folks would benefit from your Mongo OjbectId (de)serializer.
the implementation is trivial, but folks like convenience.
if you ever feel compelled to create a npm package with a bunch of (de)serializers. i'll gladly link it in the README.

e.g.

const pilerou = require('pilerou-serdes');
//...
serDes: [
  pilerou.mongodb.objectid,
  pilerou.mongodb.uuid,
  // etc,
],

[pilerou]

Hi @cdimascio (and happy new year ;))

First of all, I added a new test. If i answer with an object instead of Date and if this object hasn't a toISOString function, it fails and we get a 500 error.
I tried to add another test (and I commented it). If I answer with another object that has a toISOString function but return a string that doesn't match to date-time format, it returns 200 status code.

We can consider :

• each developer should answer with good object
• we need to control object class (instanceof ?) but it would be heavier
• we need to deserialize object and then control the return string format

I think the best solution would be the third but I don't know how we could change AJV controls order.
On requests, it should :

• control format and other jsonschema controls
• serialize to object
  On response, it shoudl :
• check if it is an object. If it is an object, we should
  • deserialize it
  • check format, pattern... or other stuff
• if response has a string
  • check format, pattern... or other stuff

Any idea ?

For the MongoDB serdes project I created it : https://www.npmjs.com/package/mongo-serdes-js
I just tested it for ObjectId feature but not yet for UUID.
As I used your uuid library, i would be happy for any advice or other formats you need to add.

[pilerou]

I just tested it on an API project with UUID :

• deserializing works on request
• Stores as UUID in Mongodb
• Serialize UUID as string

components:
  schema:
    UUID: 
      type: string,
      format: uuid,
      pattern: '^[0-9a-f]{8}-[0-9a-f]{4}-[0-5][0-9a-f]{3}-[089ab][0-9a-f]{3}-[0-9a-f]{12}$'

   app.post(`$rootUrl}/create`, async (req, res) => {
      ...
      createdUser.uuid = MUUID.v4();
      ...
      let responseContent = await myDb.Users.insert(createdUser);
      // I return created object : in db, uuid field is Binary as UUID(..) object
      res.json(responseContent.ops[0]);
      // In response, serialize function is called and UUID is stringified
   }


    app.get(`${rootUrl}/uuid/:uuid`, async (req, res) => {
      let user = await myDb.Users.findOne({ uuid });
      res.json(user);
       // in db, uuid field is Binary as UUID(..) object
       // In response, serialize function is called and UUID is stringified
   });

[cdimascio]

im struggling a bit with what the defaults should be
currently, im leaning toward the following:

• do not enable any default deserializers on request.

Why? if a request schema defines type: string, format: date-time, exisitng user code expects a string, thus if we enable the Date deserializer by default e.g. date would transform the provided string to Date on the req object. This would break exisitng user code that expects e.g. req.mydate to be a string, not a Date.

• enable date-time and date serializers by default.

Why? express will serialize the object by using its toString() method, thus users expect this behavior with the validator. Of course, the validator didn't do this until recently (a least for Dates), thue we should ensure those new Date serializers are registered by default

The unfortunate part here is that it seems a bit confusing as to what is and isn't enabled when you specify serDes.

It brings me back to something I was considering earlier, that is, should we support only serializers to start? Then see how it goes and potentially add support for deserializers later. Also, assuming, we take this approach, can we do it in a way where serializers do need to be registered and we can simply serialize with toString() for relevant cases. This might solve most cases. (though we'll likely need some registration mechanism e.g. a list of formats)

I must apologize for all the back and forth here. This is certainly complex, much more so that I initially thought.

Some of the complexities we've discovered so far:

• validation is essentially bypassed when using a serializer or deserializer is in use. ultimately, the (de)serializers must ensure that the data is valid
• backwards compatibility with deserialization is an issue (thus it must be explicitly turned on for a user)
• serialization must be on by default to more closely match express (and expected user) behavior.
• defaults become a bit more complicated since they are not symmetric. for example, when a user specifies the serDes option, what are the defaults? the default serializers should likely remain since they were on without specifying serDes, but they would need to be explicitly listed if you wanted them for serialize and deserialize. (a bit confusing O.o)

i like the (de)serialization feature, but i'm a bit hesitant as it seems it may lead to confusion and/or potentially more issue reports.

let me know if you have thoughts/suggestions.

[pilerou]

Hi Carmine,

I agree with you :

• serialization must be done automatically on Date objects when the format is date-time or date. This should be the default mecanism.
• serialization and desearialization of other formats must be optional as there's no official standard and existing projects could fail
• the addition of format, developers would want to (de)serialize must therefore be done in addition to the serialization of dates which is done by default

I therefore think that several cases should be foreseen:

Case 1

Developers want to add serialize and deserialize functions for Mongodb objects.
javascript serDes: [ mongoSerDes.objectid, mongoSerDes.uuid, ], ``
The serDes which only serialize the Date in response (without deserializing the Date in request) would be activated by defaul, in addition to those declared by developers at middleware configuration :

• dateSerializeOnly,
• dateTimeSerializeOnly

Case 2

User doesn't want serDes configuration except for Date serialization.
As in the previous case, only the default serializers are enabled.

• dateSerializeOnly,
• dateTimeSerializeOnly

Case 3

The developer wishes to deserialize the dates in request and also his MongoDB types.
javascript serDes: [ OpenApiValidator.baseSerDes.date, OpenApiValidator.baseSerDes.dateTime, mongoSerDes.objectid, mongoSerDes.uuid, ], ``
In this case, the dates are serialized and deserialized using the serDes provided in configuration which overrides the serializers set by default.

With this solution :

• there's no impact for people who doesn't want to use serDes
• it allows users who want to go forward on (de)serializing to configure their own types.

I push you in few minutes an adaptation to review.

[cdimascio]

Thanks @pilerou
I'm going to keep this in a shortly holding pattern while I try to sort out issues with the schema.preprocessor. the preprocessor is not handling all cases properly and this work depends on it working correctly. There are a couple recent bugs that exhibit the preprocessor deficiency.

[cdimascio]

@pilerou i made some improvements to the schema preprocessor. assuming preprocessor related issues settle down. i'll get back to this

[pilerou]

@cdimascio Do not hesitate if you need help or if you want me to modify the PR fork.

[cdimascio]

@pilerou thank you.
the preprocessor changes are looking good so far. there are also some significant performance improvements which is great.

as for this PR, one useful thing to do would be to rebase to mainline (or merge mainline to this branch). the recent performance (and correctness) changes impact the schema preprocessor which this of course depends on. hopefully there won't be conflicts.

[pilerou]

Hi @cdimascio
I merged in to my branch. All tests are OK and my tests projects are also OK with it.
Do not hesitate if you want me to do something more : tests, documentation...

[cdimascio]

@pilerou, the new preprocessor updaters have gone well, it seems reasonable to move forward with this
couple interface updates...

1. Update the interface to look as follows

serdes: [
  OpenApiValidator.serdes.date.serialize,  // registers serializer only
  OpenApiValidator.serdes.date.deserialize, // registers deserializer only
  OpenApiValidator.serdes.date, // registers the serializer and deserializer
],

It's possible that e.g date and/or serialize, serialize need to be methods rather than props. Feel free to explore. All in all, just want to ensure the interface is pleasant to use

2. In the README document the default behavior (and of course these options). Also, in the doc, describe that the serializer is invoked after validation, thus the serializer, thus its up to the implementor to ensure it meets validation. if there are other cavets, let's call them out in the doc. Feel free to link you serializer project here as well.

thanks!

[pilerou]

@cdimascio I changed the design and the documentation.
I also changed my mongodb serdes project https://github.com/pilerou/mongo-serdes-js
I have a last question if we want the simplest way for user. In this design, user also must add his custom formats to customFormats. It could be interesting to add them automatically as those formats are in serDes.
What do you think ?

[cdimascio]

@pilerou yes. good idea. let's add them automatically. we should call that out in the doc as well

[pilerou]

@cdimascio it's ok. I added automation on unknownFormats population using serDes formats

[cdimascio]

Were you able to modify
OpenApiValidator.baseSerDes.dateTime.serializer to OpenApiValidator.serdes.dateTime.serializer

Ultimately providing 3 options for the built-in formatters e.g.

OpenApiValidator.serdes.dateTime.serializer
OpenApiValidator.serdes.dateTime.deserializer
OpenApiValidator.serdes.dateTime

[cdimascio]

i.e. as noted in #506 (comment)

[pilerou]

Hi carmin
It's OK. I pushed a change with serdes instead of baseSerDes.

[pilerou]

Hi @cdimascio
Do you need help for something for this PR ? Maybe you want to resolve other feature before merging a new one ?
Do not hesitate if you want me to do something.

[cdimascio]

@pilerou I'm looking to get this in. I'll spend some time this weekend to get it merged. I believe you covered everything. I'll review once more this week. To be sure Apologies for the long delay.

[cdimascio]

@pilerou This change is now in!
Thanks so much for all of your efforts! Much appreciated

[cdimascio]

@pilerou i noticed this in the test output. it looks like an edge bug in serDes handling. wondering if you're up for taking a look.

The error appears when running the tests e.g. npm t

i see the following:

POST /v1/users 400 2.068 ms - 79
    ✓ should POST throw error on invalid schema Date
functionNotExists
TypeError: d.toISOString is not a function
    at Object.serialize (/Users/cdimasci/git/express-openapi-validator/src/framework/base.serdes.ts:16:19)
    at Ajv.validate (/Users/cdimasci/git/express-openapi-validator/src/framework/ajv/index.ts:90:37)
    at validate (eval at localCompile (/Users/cdimasci/git/express-openapi-validator/node_modules/ajv/lib/compile/index.js:120:26), <anonymous>:3:6690)
    at ResponseValidator._validate (/Users/cdimasci/git/express-openapi-validator/src/middlewares/openapi.response.validator.ts:179:19)
    at /Users/cdimasci/git/express-openapi-validator/src/middlewares/openapi.response.validator.ts:73:23
    at ServerResponse.json_hook (/Users/cdimasci/git/express-openapi-validator/src/framework/modded.express.mung.ts:39:16)

i see you have a test for this. that output is essentially consoled logged in the test.
it may be better to detect this case and throw a more meaningful error (rather than having it fail on the toISOString()

[pilerou]

Hello @cdimascio
I have a look to this tomorrow or tuesday and i commit a fix.

[pilerou]

And thank you for the merge. 👍

[cdimascio]

No problem. Thank you for your efforts!

[pilerou]

I had a look.
It's normal result as I see. I wanted to test an error in return type object :

express-openapi-validator/test/serdes.spec.ts

Line 197 in 4c1354f

if(req.query.baddateresponse === 'functionNotExists') {

If we want to remove this error, we juste have to remove the test here :

express-openapi-validator/test/serdes.spec.ts

Line 304 in 4c1354f

.get(`${app.basePath}/users/5fdefd13a6640bb5fb5fa925`)