pywr / pywr-next

An experimental repository exploring ideas for a major revision to Pywr using Rust as a backend.
6 stars 4 forks source link

Add #[deny_unknown_fields] to all schema structs. #165

Closed jetuk closed 2 weeks ago

jetuk commented 5 months ago

This will reduce errors by having serialisation fail on unknown fields. I think this is probably the preferred behaviour.

jetuk commented 2 months ago

This is not possible in conjunction with the "flatten" attribute. We currently use flatten for the metadata struct. See also: https://github.com/serde-rs/serde/issues/1358

Options:

  1. Leave it as is and risk silently ignoring unsupported fields.
  2. Find or wait for a workaround by reading the serde issues (none of it looks pretty at first glance)
  3. Remove the use of flatten. This would require either: a. Copying the same meta fields into each struct b. Having a nested schema like so:
{
  "type": "Input",
  "meta": {
    "name": "my-input",
    "comment": "The best input!"
  }
}

Any preferences @Batch21 @s-simoncelli @laurenpetch ?

s-simoncelli commented 2 months ago

I think having a check for unknown fields would be beneficial in case I misspell a field ☺️I wouldn’t mind having the metadata in a dedicated section where you can also keep the other less important attributes such as the coordinates or additional fields for a UI. However, the name is important because it’s a unique identifier and to me it doesn’t feel right to add it in a nested dictionary. What if we kept the metadata as separate field and used an attribute macro to add the “name” field? I have little experience with macros so I am not sure if this is the best approach; and it might not very transparent.On 30 Jul 2024, at 20:29, James Tomlinson @.***> wrote: This is not possible in conjunction with the "flatten" attribute. We currently use flatten for the metadata struct. See also: serde-rs/serde#1358 Options:

Leave it as is and risk silently ignoring unsupported fields. Find or wait for a workaround by reading the serde issues (none of it looks pretty at first glance) Remove the use of flatten. This would require either: a. Copying the same meta fields into each struct b. Having a nested schema like so:

{ "type": "Input", "meta": { "name": "my-input", "comment": "The best input!" } } Any preferences @Batch21 @s-simoncelli @laurenpetch ?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

jetuk commented 2 months ago

What if we kept the metadata as separate field and used an attribute macro to add the “name” field?

I think this is probably the best approach. We could also allow unknown fields in the meta data section so that other users/tools can use it for custom attributes?

{
  "type": "Input",
  "name": "my-input",
  "meta": {
    "comment": "The best input!"
    "position": {}
    "foo": "bar"
  }
}
s-simoncelli commented 2 months ago

I like thatOn 31 Jul 2024, at 11:23, James Tomlinson @.***> wrote:

What if we kept the metadata as separate field and used an attribute macro to add the “name” field?

I think this is probably the best approach. We could also allow unknown fields in the meta data section so that other users/tools can use it for custom attributes? { "type": "Input", "name": "my-input", "meta": { "comment": "The best input!" "position": {} "foo": "bar" } }

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>