Path or Expectation of FJS #532
Replies: 8 comments 51 replies
-
|
I want to discuss with you how FJS should work. Especially in terms of validation and output guarantees. Now FJS is a serializer with a mixin of validation checks. Some of these checks are required to define what to do with data (oneOf, anyOf, if/else). Some of these checks are not needed for serialization and check basic validation rules. There are two different points of how FJS should work.
const schema = {/*json schema*/}
const data = {/*input data*/}
// If you pass correct data it will work correctly
if (validate(data, schema)) {
const serializedData = FJS(data, schema)
const parsedData = JSON.parse(serializedData)
assert(validate(parsedData, schema)) // should always be true
}
// If you don't know that you input is valid, there is no guarantee that FJS produce valid output
const serializedData = FJS(data, schema) // might throw if can't produce the result, but doesn't have explicit validation checks
const parsedData = JSON.parse(serializedData)
assert(validate(parsedData, schema)) // unkown
const schema = {/*json schema*/}
const data = {/*input data*/}
const serializedData = FJS(data, schema) // if can't produce the correct (that will valid to the schema) output it will throw an error
const parsedData = JSON.parse(serializedData)
assert(validate(parsedData, schema)) // trueIf you have any other ideas, you can share them. |
Beta Was this translation helpful? Give feedback.
-
"It depends":
Yes, we should serialize as
Yes, there is not such feature; I though we was talking about what we would like to do in next major to be more consistent.
Yeah, I quite agree with that. The output precision should depends on the schema quality and FJS configuration
I wonder if we can open this decision to the developer |
Beta Was this translation helpful? Give feedback.
-
So that means that this comment was wrong.
Maybe I will change my mind, but I don't think it's possible. It's easier to write another package for this than maintain a strict and unstrict version in the same codebase. I understand your idea. Let's talk about the unstrict version now because it's a current working version. And I think we can't make a strict version as a default, even with a Fastify major.
I agree with this point of view. I'm not talking about the strict mode (see the previous comment). The problem is that Fastify uses FJS not just as a serializer but also as a validator for some rules like required fields. I understand the benefits, it's working pretty fast. But it creates a situation when we can't optimize FJS because we can't just remove those checks from Fastify (I 100% agree with that), and on the other hand, I, as a Fastify user, can't rely on this "validation". Is my serialized data valid to the schema after serialization? I don't know, there is no guarantee. I have to validate it to be sure. |
Beta Was this translation helpful? Give feedback.
-
I don't agree: I think the The main point I see here is that we can't apply the same rule for every keyword. Does it make sense? Example:
We are walking to the "we should throw if we can't serialize" side.
=> what is the state of the endpoint now? This is the main reason I'm PRO to the sentence:
because it is the last thing and all the business logic is done |
Beta Was this translation helpful? Give feedback.
-
|
@Eomm Sorry for the really long delay.
Here you said that if we have a value different from a constant we should serialize it and not substitute a const value. In this PR we decided to substitute a const value whatever the input is. That is why I'm saying that it was a mistake. We don't have to through an error, though. We just need to implement another logic. Instead of: const constValue = 'bar'
const serializedValue = '"bar"' // precompiled serialized const value
function serializeConst (value) {
return serializedValue // always returns const value
}we should do const constValue = 'bar'
const serializedValue = '"bar"' // precompiled serialized const value
function serializeConst (value) {
if (value === constValue) {
return serializedValue // returns const only if input match the const. fast path
}
return serialize(value)
}
I agree that keywords in the json schema spec are not "homogeneous". And originally, they were defined for validation, so we have all these problems only because we are trying to define what all these keywords should mean in the serialization process. But we can't just say ok, all keywords are different let's pick randomly some of them and apply them somehow (I'm not joking, some part of the code has no explanation). There should be logic in these definitions/serialization rules. There could be exceptions, but they also should have a reason and explanation.
Sometimes we have a situation when we don't know what to do with this data. As an option, we can always fall to the default JSON.stringifty instead of throwing an error. But it also should be defined as a part of the concept. There is a statement that I prefer: fast-json-stringify guarantees that you will get a valid output only if your input matches the schema or can be coerced to the schema. If your input doesn't match the schema, you will get undefined behavior. This means that during the compilation, we are trying to build the serialization function that expects that you pass already valid data (or data that can be coerced). If an input matches the schema, we guarantee that you will get the correct output. If the input doesn't match the schema, you will get a UB. But, unfortunately, there is only me who likes it. |
Beta Was this translation helpful? Give feedback.
-
I think there is a misunderstanding because I thought that
The logic you describe is clear, but I don't agree: why the user has set
I like this definition. I don't think we should say an "undefined behavior", but a "possibly invalid json-string if validated with the schema". I keep saying that I'm pro to the no-throw pattern |
Beta Was this translation helpful? Give feedback.
-
|
@Uzlopak Don't mind if I reply about the #520 case here.
|
Beta Was this translation helpful? Give feedback.
-
|
I think #520 actually shows the dilemma were we are walking into. See fast-json-stringify/test/const.test.js Line 237 in 1b9d5a1 So we would have stringifiying If I see it correctly #520 or a similar code change did not land in next, but stilll it makes we wonder. Also regarding type and enum: If enum is set, it is basically intersectioning with the type.
|
Beta Was this translation helpful? Give feedback.
-
|
I'm not sure which option I should vote. Based on the json schema properties list there are edge cases, and filling out this matrix will explain how FJS should/will work
|
Beta Was this translation helpful? Give feedback.
-
|
|
Beta Was this translation helpful? Give feedback.
-
|
This table doesn't work for me. I can fill this table in any way, depending on the way FJS should work. If it helps someone, you can fill it. |
Beta Was this translation helpful? Give feedback.
-
|
It helps me to understand where FJS is and how to move forward, I will try to fill it in the next days |
Beta Was this translation helpful? Give feedback.
-
|
Yes, this module is a standalone module usable by anyone to serialize data into a JSON representation. However, it is primarily developed to support Fastify. Fastify uses it to serialize response payloads by default and it is advertised to do so. The module should continue to provide the expectations that Fastify has around response serialization. That includes throwing an error when the data is missing required properties for the response to be valid. |
Beta Was this translation helpful? Give feedback.
-
|
No it’s not working as a default value. Default value will be substituted if there is no value. Null is not the absence of value. Const will not be substituted if you decide to not pass the value. |
Beta Was this translation helpful? Give feedback.
-
|
https://json-schema.org/understanding-json-schema/reference/generic.html#constant-values
So it is not part of the serialization but of the validation. So coercing { foo: null } to { "foo": "bar" } is wrong. Another example:
So you could say that const is a special case of enum. Enum with only one element can be written as const. If you use enum to which value would you coerce? And does this mean that i can avoid the coercion only by using enum with one element instead of const? |
Beta Was this translation helpful? Give feedback.
-
|
Yes, you are right, and there are a lot of examples like that. That's exactly why we have this discussion. How would you like to handle the null value in the string, const, and enum cases? |
Beta Was this translation helpful? Give feedback.
-
|
First of all: If it is type: ["string", "number"] and const is set to null then it is plain impossible to serialize the object to valid json. It is the strictness which is implied by the schema. Maybe we do some magical coercion, but then i would disable the coercion in my projects because i personally want no coercion of primitive types. And second: If i do Gold standard here is JSON.stringify. If it deviates from the output of JSON.stringify then if is not fast json stringify but something else. |
Beta Was this translation helpful? Give feedback.
-
You can't disable coercion in FJS. |
Beta Was this translation helpful? Give feedback.
-
|
fast-json-stringify was designed to be a serializer. However, I added some basic validation, like Performing full validation would slow it to a halt, make it useless. Some validation is also needed to support all the JSON Schema features. |
Beta Was this translation helpful? Give feedback.
-
|
I made the PRs #611 and #579 to get some ground to talk. But to be frank, i am quite dissapointed as there is no clear goal to reach. |
Beta Was this translation helpful? Give feedback.
-
|
Tables help in this case - I didn't fulfil yet as I don't have a fresh mind with the actual source code |
Beta Was this translation helpful? Give feedback.
-
|
It might be good to design a pluggable compiler. So anyone could write his own plugin and inject some code into the precompiled serialization function. After that, we could insert into the code any validation or type coercion without modifying the compiler core module. const compiler = new Compiler({
preArraySerializationHook: (ctx, array, arraySchema) => {
if (array.length !== arraySchema.lenght) {
throw new Error ('Array has wrong length')
}
},
preIntegerSerializationHook: () => {},
preObjectSerializationHook: () => {},
// ...
preSchemaSerializationHooks: {
'schema-1': () => {},
'schema-2': () => {},
}
})
const serialize = compiler.build(schema, {
// pre serialization hooks only for this schema
})
serialize(object)P.S. I'm not sure that it's possible to write it, but I would try. |
Beta Was this translation helpful? Give feedback.
-
|
Currently: If we have a schema: If we have a schema: If we have a schema: If we have a schema: ( This existing behaviour seems inconsistent and surprising. I don't think fast-json-stringify should ever do a type coercion that That is, I think that the design goal of FJS should be a new option 3:
Properties missing from the value that are in the schema should always be omitted (even if the schema defines a default or const), and the result will always match the parts of the schema for which data is returned. I don't think FJS should ever throw on a value that does not match the schema (including in the cases where it currently does throw, like providing an object for a number). If you want full validation, just validate your value with AJV before stringifying it. But this raises a problem with anyOf/allOf/if-else: in these cases. For example, if we have: schema: What should Serialising these constructs is always going to be slower, because we need to check which option to proceed with before doing the serialisation. So the easy option would just be to give up and call I think it would be reasonable to instead restrict the ways that anyOf, oneOf, allOf, if-else can be used in schemas that are to be used with fast-json-stringify:
These properties of the schema should be checked at compile time, and ignored or throw, depending on a strict option. If they are ignored, the corresponding parts of a schema would be omitted from serialisation (not just delegated to JSON.stringify). So for example a property with a oneOf subschema without a discriminator keyword would always be omitted from serialisation, regardless of the value passed, or would throw at compile time if compiled with a strict option. I think this approach would provide an easy to understand, consistent, fast, and safe* way of serialising data, and clearly defines what fast-json-stringify does (serialisation), and does not do (validation of the data to be serialised). *The safety that I think matters most in the context of serialising data for an API is making sure that only data that the programmer intends to be serialised is serialised (preventing unexpected data leaking in an API). I appreciate that this approach would be a big change from the current behaviour, but having poked around fast-json-schema a bit recently, I thought this view of things might be useful, and this thread seemed like the place for this discussion. |
Beta Was this translation helpful? Give feedback.
-
|
@autopulated Thanks for your feedback. Before discussing implementation details we should understand can we do this and if yes, then how we can do this. I aslo would like to have a consistent and simple idea of how this library should work. Your idea of making it work exactly as JSON.stringify is consistent and simple, although I don't think it will work for a coumple of reasons.
|
Beta Was this translation helpful? Give feedback.
-
|
No worries, thanks for explaining, totally understand all of your points. |
Beta Was this translation helpful? Give feedback.
-
Within the discussion inside #530.
I believe we are lost on the path or expectation of this module.
It is clear that we divide into two separate opinion.
FJS should be a plain serializer, we shouldn't care on the validation.
FJS should be a serializer that return the
valuecan be validate against theJSON-Schema.I open a poll here to helps us make decision on which path should we go.
3 votes ·
Beta Was this translation helpful? Give feedback.
All reactions