Add support for ES6 Set #545

TommyDew42 · 2022-09-13T16:06:11Z

What this PR does

An issue from #301.

We add support for Set in this PR only. A new Set([1, 2, 3, 4]) will be stringified to [1,2,3,4]

Unit tests:

a set of one type, for each primitive type, bigint & date-time.
a set of mixed types
an empty set
a nested set
a set as a constant
an object with toJSON and a set
if-then-else
array keyword like maxItems, contains, minContains

PR for `Map`

Working on Map now. Instead of having map and set feature in one PR, we have a separate PR for Map later?

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

ivan-tymoshenko · 2022-09-13T16:13:25Z

Please make sure that validation works with Map, Set. By validation I mean cases when we've got type property as an array, oneOf, anyOf, if/then/else schema.

TommyDew42 · 2022-09-13T16:17:52Z

Thanks! Let me add unit tests for them tmr! @ivan-tymoshenko

ivan-tymoshenko · 2022-09-13T16:25:22Z

Thanks! Let me add unit tests for them tmr! @ivan-tymoshenko

It wouldn't be just test cases. You will have to make Ajv works with Map and Set as an 'object' type. If you have questions ask me I will tell you where to loot at. But it's not so easy to add support of two new types. I'm not sure if it worth it.

TommyDew42 · 2022-09-13T16:29:33Z

🤔 I see... we have to update build-schema-validator.js. How does the validation works in a high level understanding?

ivan-tymoshenko · 2022-09-13T16:37:10Z

You need to make this test works.

test('should stringify Set as an array', (t) => {
  t.plan(2)

  const schema = {
    anyOf: [
      { type: 'array', items: { type: 'string' } },
      { type: 'object', properties: { foo: { type: 'string' } } }
    ]
  }

  const stringify = build(schema)
  t.equal(stringify(new Set(['foo', 'bar'])), '["foo","bar"]')
  t.equal(stringify({ foo: 'bar' }), '{"foo":"bar"}')
})

mcollina

lgtm

Eomm · 2022-09-14T06:01:52Z

test/const.test.js

+
+  const stringify = build(schema)
+  const output = stringify({
+    constantSet: new Set([2, 3]), nonConstantSet: new Set(['Hi', 'it\'s', 'Tommy'])


Suggested change

constantSet: new Set([2, 3]), nonConstantSet: new Set(['Hi', 'it\'s', 'Tommy'])

constantSet: new Set([4, 5, 6]),

nonConstantSet: new Set(['Hi', "it's", 'Tommy aka "Tom"'])

Eomm · 2022-09-14T06:08:39Z

test/const.test.js

+  })
+
+  t.same(
+    "{\"constantSet\":[2,3],\"nonConstantSet\":[\"Hi\",\"it's\",\"Tommy\"]}",


Suggested change

"{\"constantSet\":[2,3],\"nonConstantSet\":[\"Hi\",\"it's\",\"Tommy\"]}",

"{\"constantSet\":[2,3],\"nonConstantSet\":[\"Hi\",\"it's\",\"Tommy aka \\"Tom\\"\"]}",

ivan-tymoshenko

Validation should work with new types. I described above.

TommyDew42 · 2022-09-17T20:01:34Z

I'm working on the validation and have a new idea. Maybe it's easier to change the type in the schema from array to set:

const schema = {
  title: 'a set of booleans',
  type: 'array',
  items: { type: 'boolean' }
},
const data = new Set([false, true]),

So we can internally convert the type in the validator to object. The conversion is similar to what we have been doing with fjs_type here.

Of course we can also have the conversion if we keep the type as what we are doing in this PR: the type is simply array. But the conversion would be to ['array', 'object'] since we don't know if it would be a set or array when the type being array.

TommyDew42 · 2022-09-19T16:35:38Z

@ivan-tymoshenko @mcollina @Eomm Wanna get you guys thoughts on using set as the type declaration on the schema instead of array

TommyDew42 · 2022-09-24T17:37:46Z

Going to implement this with set instead of array as the type declaration in the schema if no objection from you guys.
@ivan-tymoshenko @mcollina @Eomm

ivan-tymoshenko · 2022-09-24T17:55:10Z

Going to implement this with set instead of array as the type declaration in the schema if no objection from you guys.

@ivan-tymoshenko @mcollina @Eomm

I haven’t checked it yet. I will have some time tomorrow. We can discuss it.

Uzlopak · 2022-09-24T18:54:57Z

If i understand this correctly you want to implement a new type. Something like

{ 
  type: "set"
}

instead of

{
  type: "array"
}

Tbh i dont agree. type: "set" is not a valid json schema 7 type. The whole scope of this ES6 support is to implicitly serialize an ES6 Set as an Array. Keep in mind there is no way to deserialize a jsonstring to a Set. So there is information loss. But for that cases you would define a reviver for JSON.parse, which would revive the Set from the Array.

So again: I totally disagree with adding a non-standard json-schema type.

Eomm · 2022-09-25T07:20:03Z

So again: I totally disagree with adding a non-standard json-schema type.

Yes, I'm with you.

This would break everything:

swagger
open api
fluent-schema and every JSON schema builder

It would not add any value to the user

…to add-support-for-set

TommyDew42 · 2022-09-25T16:05:37Z

Thanks for reminding me of json schema 7 type!
As what Ivan said, it's not so easy to add support of two new types validation.
The tricky thing is that the validator treats Set as an object while we want it to act like an array.

I take a look at json schema 7 type and noticed there's an uniqueItems keyword. I don't think we are using the keyword:

const stringify = build({ type: 'array', uniqueItems: true })
console.log(stringify([1, 1, 2])) // "[1,1,2]"

Would using { type: 'array', uniqueItems: true } to indicate Set be a better idea?

Uzlopak · 2022-09-25T21:44:24Z

What is the purpose to "indicate" a Set when uniqueItems is set to true? What is there to indicate?

Eomm · 2022-09-26T06:58:37Z

the uniqueItems option has been added in Draf 8 version of the JSON Schema. So we can't relay on it.

I thought the issue here was:

When a user wants to serialize a Set in a type: Array field, the Set will be serialized as a primitive Array.

So, I don't get the point where the user must declare that it wants to support the Set.
It should be a transparent feature/fix to me supporting the Set object.

Am I wrong?

…to add-support-for-set

TommyDew42 · 2022-09-27T16:59:58Z

Thanks for @Uzlopak and @Eomm's input! The reason that I want the user to indicate they are stringifying a set is simple. I was struggling with the set validation and the indication helps with the implementation 😿.

But luckily I realised we can just follow how we are dealing with fjs_type for set validation. Two tests for anyOf and oneOf was added in 7276091

There's one limitation tho. The if-else-then doesn't work on Set:

t.test('if/else with set', (t) => {
  t.plan(2)

  const schema = {
    type: 'array',
    if: { type: 'array', maxItems: 1 },
    then: { items: { type: 'string' } },
    else: { items: { type: 'number' } }
  }

  const stringify = build(schema)

  t.equal(stringify(new Set(['1'])), JSON.stringify(['1']))
  t.equal(stringify(new Set(['1', '2'])), JSON.stringify([1, 2])) // fail and stringified to '["1","2"]'
})

I am not sure the simple way to solve this... 🥵 Shall we simply write down the limitation?

Uzlopak · 2022-09-27T17:09:06Z

@ivan-tymoshenko
We need your skills here

Eomm · 2022-09-27T19:12:07Z

You need one more check

diff --git a/index.js b/index.js
index 8939e92..275e9ab 100644
--- a/index.js
+++ b/index.js
@@ -475,6 +475,9 @@ function addIfThenElse (location, input) {
   elseLocation.schema = merge(schema, elseSchema)
 
   return `
+    if (${input} instanceof Set) {
+      ${input} = Array.from(${input})
+    }
     if (validator.validate("${ifSchemaRef}", ${input})) {
       ${buildValue(thenLocation, input)}
     } else {

ivan-tymoshenko · 2022-09-28T07:50:44Z

lib/validator.js

+      type: 'object',
+      errors: false,
+      validate: (type, data) => {
+        return data instanceof Set


I didn't check the whole PR yet. But I think it's not enough here. You have to check that all restrictions like items, minItems, maxItems will work in this case.

…dation

TommyDew42 · 2022-09-28T16:08:16Z

Commit a400d84 will allow us to do all array validations on Set. Unit tests in 8c1d990.

But there's one more limitation tho.
It doesn't work when both object and array are in schema type: { type: ['object', 'array'] }

ivan-tymoshenko · 2022-09-28T16:13:52Z

lib/validator.js

+          schema.type.pop('object')
+        }
+
+        return this.ajv.validate(schema, data)


It will work if this schema is a Set, but it won't work for any nested schemas.

Do we have an example of the nested schema?

const schema = { anyOf: [ { type: 'string' } { type: 'object', properties: { a: { type: 'array', items: { type: 'number' } } } } ] } const data1 = 'foo' const data2 = { a: new Set([1, 2, 3]) }

I'm guessing it will also work for nested schemas. It's because the custom isSet keyword validate function is not only called on the top level but every level on the schema if isSet is presented.

Added a few unit tests for nested schema in 47a0871 & 8c1d990!

ivan-tymoshenko · 2022-09-28T16:15:17Z

lib/validator.js

+          // undo what we do in convertSchemaToAjvFormat
+          // to prevent infinite recursive call
+          delete schema.isSet
+          schema.type.pop('object')


array's pop method always returns the last item and doesn't have any arguments

In 7d169ea! But also found that the JSON.stringify inside the error message doesn't handle Set well. Can we please have a separate PR for that?

TommyDew42 · 2022-09-28T16:54:53Z

But I found a potential bug if I understand if-then-else logic correctly.
It seems that error is because the anyof validator.validate receive { a:[] } instead of the object after modified with const value

test('anyOf with string and array', (t) => {
  t.plan(2)

  const schema = {
    anyOf: [
      { type: 'string' },
      {
        type: 'object',
        properties: {
          a: {
            type: 'array',
            if: { items: { type: 'number' }, minItems: 1 },
            then: { items: { type: 'number' } },
            else: { const: ['const item'] }
          }
        }
      }
    ]
  }

  const stringify = build(schema)

  t.equal(stringify('foo'), '"foo"') // ok
  t.equal(stringify({ a: [1, 2, 3] }), JSON.stringify({ a: [1, 2, 3] })) // ok
  t.equal(
    stringify({ a: [] }),
    JSON.stringify({ a: ['const item'] })
  ) // throw error: The value {"a":[]} does not match schema definition.
})

ivan-tymoshenko · 2022-09-28T16:57:37Z

lib/validator.js

+          schema.type = schema.type.filter(type => type !== 'object')
+        }
+
+        return this.ajv.validate(schema, data)


I'm not sure that it will work with local references inside the schema.

Do we have an example?

const schema = { anyOf: [ { type: 'string' } { type: 'object', properties: { a: { type: 'array', items: { $ref: '#/anyOf/0' } } } } ] }

Nah It doesn't work :(. The schema arg in the validate function would just be { type: [ 'array' ], items: { '$ref': '#/anyOf/0' } }.

Ugh let me think again. Please also let me know if you got any ideas.

Uzlopak · 2022-09-29T08:23:38Z

Shouldnt we just convert the Set to an Array and then the normal array validation takes over?

I have the feeling the current implementation is overcomplicated and does not what it wants to achieve. E.g. why do i need a keyword isSet (despite it should be named fjs_set or so) if I convert everything to an array anyway. Ajv can not iterate over the Set anyway.

Or what do you think?

TommyDew42 · 2022-09-29T11:32:25Z

Do you mean turning all the set in the object passed in? That’s right… this should be a way simpler solution. Would it have performance issue? We are traversing the entire object.

I can try that.

Eomm · 2022-09-29T19:30:26Z

if I convert everything to an array anyway

That is needed because we use ajv to apply the oneOf and anyOf validations. So that code is needed by ajv.

The other statements are needed to manage the code branches we generate through the building phase of the serialization function.

I have the feeling the current implementation is overcomplicated

The solution is not generic, but it is complete and simple. Unfortunately, we have many use cases (const, any/oneOf, plain schemas without root object)

I'm OK with this approach.
Other refactors should be on us IMHO

ivan-tymoshenko · 2022-09-29T19:48:53Z

The solution is not generic, but it is complete and simple. Unfortunately, we have many use cases (const, any/oneOf, plain schemas without root object)

I didn't get this point. At the beginning of this issue, I said that adding a new unsupported type wouldn't be easy. I'm not ok with adding only the serialization part without validation.

ivan-tymoshenko · 2022-09-29T19:58:20Z

Do you mean turning all the set in the object passed in? That’s right… this should be a way simpler solution. Would it have performance issue? We are traversing the entire object.

I can try that.

If you want to make a simple solution, it will work. Of course, it will have some performance drop. Plus, remember that you shouldn't modify the original object. This means you will have to clone some part of it, which also takes some time.

Uzlopak · 2022-09-29T20:16:18Z

If you want to iterate over the Set etc.. I am also fine. Then implementing it needs to be a full solution and not something which works only partially.

TommyDew42 · 2022-10-01T02:22:44Z

I guess what we are still missing in this PR is just Set validation with local reference. We have had all other features. Shall we try to get the validation with local reference first?

It seems to me turning all Set to array brings overhead to all cases where Set is not involved. And this might not be ideal if people don't have a frequent need to stringify Set. Maybe it's better to let people do the conversion from Set to array instead (?) But I'm not 100% sure about the overhead.

But feel free to let me know what your thoughts are. Even if you think the whole PR is unnecessary.

Eomm · 2022-10-01T07:12:53Z

remember that you shouldn't modify the original object

Instead of replacing the value, we could add a symbol with the Array to access later

ivan-tymoshenko · 2022-10-01T12:54:19Z

Instead of replacing the value, we could add a symbol with the Array to access later

You still will have to make some checks during serialization/validation. If I understand your idea correctly, it won't help.

TommyDew42 · 2022-10-07T15:40:15Z

I encountered some problems about cloning the object prototype methods when I tried to implement the convert-set-to-array solution. This seems to be a limitation/feature of our clone library (See davidmarkclements/rfdc#29).

I didn't expect this issue would be this complicated. Would like to take a break from it first. Feel free to tackle this when you are interested! 😸 Really appreciate the help from Ivan, Uzlopak & Manuel.

TommyDew42 added 7 commits September 13, 2022 23:43

stringify set as an array

e853f76

unit tests for set

cb0aabb

add set documentation

c8835bf

remove comments in set.test.js

eaa5797

add items type to set unit tests

9bc426f

add a unit test for a set of dates

d9f582f

correct grammar in readme

c056c07

mcollina requested review from ivan-tymoshenko and Eomm September 13, 2022 20:23

mcollina approved these changes Sep 13, 2022

View reviewed changes

Eomm reviewed Sep 14, 2022

View reviewed changes

Uzlopak changed the title ~~Add support for set~~ Add support for ES6 Set Sep 14, 2022

ivan-tymoshenko requested changes Sep 14, 2022

View reviewed changes

delete map.test.js

78c936e

Merge branch 'master' of github.com:TommyDew42/fast-json-stringify in…

fe97b83

…to add-support-for-set

let validator support ES6 Set in anyOf and oneOf

04e8fe7

Merge branch 'master' of github.com:TommyDew42/fast-json-stringify in…

f779391

…to add-support-for-set

ivan-tymoshenko requested changes Sep 28, 2022

View reviewed changes

TommyDew42 added 4 commits September 28, 2022 23:59

correct the order in t.same

8072d3c

change the validate logic for isSet keyword to allow complicated vali…

a400d84

…dation

add unit tests for if-then-else & array keywords for Set

8c1d990

add array keyword to the Set example

2e62947

ivan-tymoshenko reviewed Sep 28, 2022

View reviewed changes

TommyDew42 added 2 commits September 29, 2022 00:46

add one more anyOf test for Set

47a0871

correct how we pop object from schema.type

5119725

ivan-tymoshenko reviewed Sep 28, 2022

View reviewed changes

one more anyOf test where set doesn't follow schema

7d169ea

TommyDew42 closed this Oct 7, 2022

This was referenced Oct 7, 2022

Any plans to add support replacer functionality? #301

Closed

Object should pass validation after it is modified by if-then-else logic. #550

Open

	constantSet: new Set([2, 3]), nonConstantSet: new Set(['Hi', 'it\'s', 'Tommy'])
	constantSet: new Set([4, 5, 6]),
	nonConstantSet: new Set(['Hi', "it's", 'Tommy aka "Tom"'])

	"{\"constantSet\":[2,3],\"nonConstantSet\":[\"Hi\",\"it's\",\"Tommy\"]}",
	"{\"constantSet\":[2,3],\"nonConstantSet\":[\"Hi\",\"it's\",\"Tommy aka \\"Tom\\"\"]}",

Add support for ES6 Set #545

Add support for ES6 Set #545

Conversation

TommyDew42 commented Sep 13, 2022 • edited Loading

What this PR does

Unit tests:

PR for Map

Checklist

ivan-tymoshenko commented Sep 13, 2022

TommyDew42 commented Sep 13, 2022

ivan-tymoshenko commented Sep 13, 2022

TommyDew42 commented Sep 13, 2022

ivan-tymoshenko commented Sep 13, 2022

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivan-tymoshenko left a comment

Choose a reason for hiding this comment

TommyDew42 commented Sep 17, 2022

TommyDew42 commented Sep 19, 2022

TommyDew42 commented Sep 24, 2022

ivan-tymoshenko commented Sep 24, 2022

Uzlopak commented Sep 24, 2022 • edited Loading

Eomm commented Sep 25, 2022

TommyDew42 commented Sep 25, 2022 • edited Loading

Uzlopak commented Sep 25, 2022

Eomm commented Sep 26, 2022

TommyDew42 commented Sep 27, 2022 • edited Loading

Uzlopak commented Sep 27, 2022

Eomm commented Sep 27, 2022

Choose a reason for hiding this comment

TommyDew42 commented Sep 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TommyDew42 Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TommyDew42 Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

TommyDew42 commented Sep 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uzlopak commented Sep 29, 2022

TommyDew42 commented Sep 29, 2022

Eomm commented Sep 29, 2022

ivan-tymoshenko commented Sep 29, 2022

ivan-tymoshenko commented Sep 29, 2022

Uzlopak commented Sep 29, 2022

TommyDew42 commented Oct 1, 2022

Eomm commented Oct 1, 2022

ivan-tymoshenko commented Oct 1, 2022

TommyDew42 commented Oct 7, 2022 • edited Loading

TommyDew42 commented Sep 13, 2022 •

edited

Loading

PR for `Map`

Uzlopak commented Sep 24, 2022 •

edited

Loading

TommyDew42 commented Sep 25, 2022 •

edited

Loading

TommyDew42 commented Sep 27, 2022 •

edited

Loading

TommyDew42 Sep 28, 2022 •

edited

Loading

TommyDew42 Sep 28, 2022 •

edited

Loading

TommyDew42 commented Oct 7, 2022 •

edited

Loading