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

Runtime execution over missing fields when field is optional #197

Closed
jeromesimeon opened this issue Dec 22, 2018 · 5 comments
Closed

Runtime execution over missing fields when field is optional #197

jeromesimeon opened this issue Dec 22, 2018 · 5 comments

Comments

@jeromesimeon
Copy link
Member

The current JavaScript backend code generation expects JSON with a field containing null for optional fields in the CTO model.

So for a concept:

concept C{
  o Integer a optional
}

The expected serialization is currently:

{ "$class" : "C", "a" : null }

This is inconsistent with the way Concerto validates optional fields, instead assuming the default serialization is:

{ "$class" : "C" }

It would be useful to support both serialization in Ergo.

Note that it might be less confusing to Ergo user is the default Concerto serialization use the null form, since in Ergo, you always have to write:

C{ a: none }
@jeromesimeon
Copy link
Member Author

jeromesimeon commented Jul 16, 2020

Status: both serializations now work in Ergo. This leaves the questions about how we present the data model: (serialization/deserialization) to users for:

  • In concerto validation: null in fields or fields that are absent / fields with a content or fields that are present
  • how we present the same thing in the Ergo data model and type system: none / some

So in effect Ergo takes the interpretation that the data model is a field containing either null or something, not that the field itself is present or absent.

I would love it if we could align the interpretation (changing that in Ergo is rather non-trivial, so my preference would be to switch the interpretation in Concerto).

I am moving this discussion to the Concerto repo.

@jeromesimeon
Copy link
Member Author

jeromesimeon commented Jul 16, 2020

@dselman @mttrbrts Would love to have your thoughts on this issue. I would love to sneak in this change into Concerto 1.0.

Concretely the change would be, for the Concerto model:

concept C{
  o Integer a optional
  o String b
}

Current behaviour:

validate { "$class" : "C", "b" : "Foo " }             -> { "$class" : "C", "b" : "Foo " }
validate { "$class" : "C", "a" : null, "b" : "Foo " } -> { "$class" : "C", "b" : "Foo " }

New behaviour:

validate { "$class" : "C", "b" : "Foo " }             -> { "$class" : "C", "a": null, "b" : "Foo " }
validate { "$class" : "C", "a" : null, "b" : "Foo " } -> { "$class" : "C", "a": null, "b" : "Foo " }

Some beliefs:

  • For the purpose of validation etc. I don't think this changes anything fundamental.
  • For Ergo it doesn't change anything, but the serialization is more consistent with the user code (e.g., users write C{ a: none, b: "Foo" })
  • For JavaScript, truthy tests like if (validated.a) { ... } keep their semantics, but some other tests, e.g., if (validated.a === null) { ... } may change
  • For code generation (e.g., in Java) I think this is also more consistent (to be checked)

@dselman
Copy link
Contributor

dselman commented Jul 17, 2020

If I’ve understood the proposal, you’d like all optionals to be serialised as ‘null’ when absent in the JSON?

Some questions:

  • will impact size of the documents (many AP models have many optionals)
  • with the serialiser going away post v1 where would we do this? Using the Concerto API we validate the user JS object as-is. It seems awkward to have to specify null for potentially dozens of optionals.
  • this seems stricter in terms of model evolution, in that removing an optional now breaks all instances. Not sure that is bad, but it is interesting.

@jeromesimeon
Copy link
Member Author

jeromesimeon commented Jul 25, 2020

If I’ve understood the proposal, you’d like all optionals to be serialised as ‘null’ when absent in the JSON?

Some questions:

  • will impact size of the documents (many AP models have many optionals)
  • with the serialiser going away post v1 where would we do this? Using the Concerto API we validate the user JS object as-is. It seems awkward to have to specify null for potentially dozens of optionals.
  • this seems stricter in terms of model evolution, in that removing an optional now breaks all instances. Not sure that is bad, but it is interesting.

Hm.. interesting questions.

  1. Size of the JSON data -- I think this is in the noise, maybe I'm wrong? Do we really care about this level of optimisation?
  2. very very good question. I think this is the biggest one. Can we really get away without having some changes to the JSON post validation? (e.g., default values from the model?) If yes, then we might want to consider this quite carefully (or maybe it just becomes a moot point -- note we will still need a JSON -validate+convert-> ErgoJSON / other things path for Ergo (and different representations e.g., WASM). but that could be some other operation.
  3. symmetrically, currently changing an optional to a non optional would break? I think this shifts what kind of evolution would be transparent or not... I hadn't thought of that.

@jeromesimeon
Copy link
Member Author

This makes me think we could revisit this / leaves this open until we have a better idea of the "functional validation" change.

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

3 participants