add `schemars` to `ZeroVec` #4792

ashu26jha · 2024-04-10T16:03:27Z

This PR intends to add schemars support to ZeroVec. Implemented JsonSchema for following:

ZeroVec
VarZeroVec
ZeroSlice
ZeroVecError

Added a basic test, just to check the generation. The JSON generated from the test:

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "title": "DataStruct",
  "type": "object",
  "required": [
    "chars",
    "nested_numbers",
    "nums",
    "strs"
  ],
  "properties": {
    "chars": {
      "$ref": "#/definitions/ZeroVec<Character>"
    },
    "nested_numbers": {
      "$ref": "#/definitions/VarZeroVec<ZeroSlice<uint32>>"
    },
    "nums": {
      "$ref": "#/definitions/ZeroVec<uint32>"
    },
    "strs": {
      "$ref": "#/definitions/VarZeroVec<String>"
    }
  },
  "definitions": {
    "VarZeroVec<String>": {
      "type": "array",
      "items": {
        "type": "string"
      }
    },
    "VarZeroVec<ZeroSlice<uint32>>": {
      "type": "array",
      "items": {
        "$ref": "#/definitions/ZeroSlice<uint32>"
      }
    },
    "ZeroSlice<uint32>": {
      "type": "object",
      "required": [
        "data"
      ],
      "properties": {
        "data": {
          "type": "array",
          "items": {
            "type": "integer",
            "format": "uint8",
            "minimum": 0.0
          }
        },
        "error": {
          "$ref": "#/definitions/ZeroVecError"
        }
      }
    },
    "ZeroVec<Character>": {
      "type": "array",
      "items": {
        "type": "string",
        "maxLength": 1,
        "minLength": 1
      }
    },
    "ZeroVec<uint32>": {
      "type": "array",
      "items": {
        "type": "integer",
        "format": "uint32",
        "minimum": 0.0
      }
    },
    "ZeroVecError": {
      "description": "ZeroVecError is an enum representing errors that can occur during the decoding of slices of ULE",
      "type": "string",
      "enum": [
        "InvalidLength",
        "ParseError",
        "VarZeroVecFormatError"
      ]
    }
  }
}

Marking this part of #2118, as it may require more work.

Implement JsonSchema:

ashu26jha · 2024-04-10T16:05:07Z

utils/zerovec/src/zerovec/slice.rs

+    fn json_schema(gen: &mut SchemaGenerator) -> Schema {
+        // Instead of generating a subschema for T, we generate a schema representing the byte array
+        let byte_schema = gen.subschema_for::<Vec<u8>>();
+
+        SchemaObject {
+            instance_type: Some(InstanceType::Object.into()),
+            object: Some(Box::new({
+                let mut object_validation = schemars::schema::ObjectValidation::default();
+                object_validation
+                    .properties
+                    .insert("data".to_string(), byte_schema);
+                object_validation
+                    .properties
+                    .insert("error".to_string(), gen.subschema_for::<ZeroVecError>());
+                object_validation.required.insert("data".to_string());
+                object_validation
+            })),
+            ..Default::default()
+        }
+        .into()
+    }
+}


@ maintainers I am not certain if this is the right way to move forward

Manishearth · 2024-04-10T20:41:33Z

utils/zerovec/Cargo.toml

@@ -37,6 +38,7 @@ serde = { version = "1.0", default-features = false, features = ["alloc"], optio
 # and all 0.7 versions, but not further.
 yoke = { version = ">=0.6.0, <0.8.0", path = "../yoke", optional = true }
 twox-hash = { version = "1.6", default-features = false, optional = true }
+schemars = "0.8.16"


issue: this must be an optional dependency

Yes, and please add it to the top-level Cargo.toml (ideally wait until #4844 is merged)

Manishearth · 2024-04-10T20:41:46Z

utils/zerovec/src/error.rs

@@ -5,6 +5,19 @@
 use core::any;
 use core::fmt;

+use crate::__zerovec_internal_reexport::boxed::Box;


issue: just alloc::boxed::Box

Manishearth · 2024-04-10T20:45:46Z

utils/zerovec/src/zerovec/slice.rs

+
+    fn json_schema(gen: &mut SchemaGenerator) -> Schema {
+        // Instead of generating a subschema for T, we generate a schema representing the byte array
+        let byte_schema = gen.subschema_for::<Vec<u8>>();


question: why? ZeroSlice is logically an array of items

Yeah, ZeroVec and ZeroSlice should both become arrays of T.

Maybe they can resolve to the same JsonSchema impl.

Agreed, expected interaction should be as array not bytes. Lets fallback to implementing as arrays of T.

sffc

Thanks for looking into this!

sffc · 2024-04-12T06:17:35Z

utils/zerovec/src/lib.rs

+        nested_numbers: VarZeroVec<'data, ZeroSlice<u32>>,
+    }
+
+    #[test]


Suggestion: use insta to help generate test cases and check-in the JSON Schema output.

Thanks, will look into it 👍

sffc · 2024-04-12T06:19:51Z

utils/zerovec/src/zerovec/slice.rs

+
+    fn json_schema(gen: &mut SchemaGenerator) -> Schema {
+        // Instead of generating a subschema for T, we generate a schema representing the byte array
+        let byte_schema = gen.subschema_for::<Vec<u8>>();


Yeah, ZeroVec and ZeroSlice should both become arrays of T.

Maybe they can resolve to the same JsonSchema impl.

sffc · 2024-04-12T06:21:10Z

utils/zerovec/src/lib.rs

+        let schema = gen.into_root_schema_for::<DataStruct>();
+        let schema_json =
+            serde_json::to_string_pretty(&schema).expect("Failed to serialize schema");
+        println!("{}", schema_json);


Nit: Please include the generated JSON.

sffc · 2024-04-12T06:23:19Z

utils/zerovec/src/lib.rs

+
+    #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+    #[derive(JsonSchema)]
+    pub struct DataStruct<'data> {


Suggestion: for in-file unit tests, make one test for each of these. For example,

#[test] fn test_schema_zerovec_u32() { let gen = SchemaGenerator::default(); let schema = gen.into_root_schema_for::<ZeroVec<u32>>(); // ... }

As suggested below, use insta for the more complex tests.

sffc · 2024-04-12T06:27:03Z

  "properties": {
    "chars": {
      "$ref": "#/definitions/ZeroVec<Character>"
    },
    "nested_numbers": {
      "$ref": "#/definitions/VarZeroVec<ZeroSlice<uint32>>"
    },
    "nums": {
      "$ref": "#/definitions/ZeroVec<uint32>"
    },
    "strs": {
      "$ref": "#/definitions/VarZeroVec<String>"
    }
  },

While technically correct, I think I would expect these to be inlined at the call site rather than being stuffed into the definitions section. I imagine the definitions section being useful when you have deeply nested structs or a struct that gets used multiple times, but these vecs should just be vecs.

Is this configurable in the JsonSchema generator, or is it a result of the JsonSchema trait impl?

sffc · 2024-04-12T06:30:36Z

In other words, I think my expectation for the JSON Schema of your sample struct would be

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "title": "DataStruct",
  "type": "object",
  "required": [
    "chars",
    "nested_numbers",
    "nums",
    "strs"
  ],
  "properties": {
    "chars": {
      "type": "array",
      "items": {
        "type": "string",
        "maxLength": 1,
        "minLength": 1
      }
    },
    "nested_numbers": {
      "type": "array",
      "items": {
        "type": "array",
        "items": {
          "type": "integer",
          "format": "uint32",
          "minimum": 0.0
        }
      }
    },
    "nums": {
      "type": "array",
      "items": {
        "type": "integer",
        "format": "uint32",
        "minimum": 0.0
      }
    },
    "strs": {
      "type": "array",
      "items": {
        "type": "string"
      }
    }
  }
}

ashu26jha · 2024-04-12T18:22:54Z

While technically correct, I think I would expect these to be inlined at the call site rather than being stuffed into the definitions section. I imagine the definitions section being useful when you have deeply nested structs or a struct that gets used multiple times, but these vecs should just be vecs.
Is this configurable in the JsonSchema generator, or is it a result of the JsonSchema trait impl?

@sffc Found something in the docs and in thier codebase:

What does docs mentions: is_referenceable() returns false for simpler schemas, but for complex/recursive ones it returns true. But the schema I tested is probably the simpliest of our use cases, not sure why we are getting references. is_referenceable(), $ref

What does code mentions: Under SchemaSettings we do indeed have inline_subschemas by default this is false, we can set this to true. Few references still be generated for recursive type (We do want to keep this).

I could give setting inline_subschemas to true a shot and see how it pans out.

In other words, I think my expectation for the JSON Schema of your sample struct would be

Yes indeed, myself not a fan of very big schemas. Having schemas too big basically defies the purpose.

ashu26jha · 2024-04-13T12:31:43Z

@sffc @Manishearth

New JSON, I was able to generate with setting inline_subschemas to true :

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "title": "DataStruct",
  "type": "object",
  "required": [
    "chars",
    "nested_numbers",
    "nums",
    "strs"
  ],
  "properties": {
    "chars": {
      "type": "array",
      "items": {
        "type": "string",
        "maxLength": 1,
        "minLength": 1
      }
    },
    "nested_numbers": {
      "type": "array",
      "items": {
        "type": "array",
        "items": {
          "type": "integer",
          "format": "uint32",
          "minimum": 0.0
        }
      }
    },
    "nums": {
      "$ref": "#/definitions/ZeroVec<uint32>"
    },
    "strs": {
      "type": "array",
      "items": {
        "type": "string"
      }
    }
  }
}

Which one should we go for?

ashu26jha · 2024-04-16T04:08:57Z

Update the JSON, if all looks good, I think we are in position to write for ZeroMap & ZeroMap2d as well

sffc

Please address the open feedbacks.

sffc · 2024-05-16T03:54:11Z

utils/zerovec/Cargo.toml

@@ -24,6 +24,7 @@ all-features = true

 [dependencies]
 zerofrom = { workspace = true }
+serde_json = "1.0"


Issue: Use the { workspace = true } version of the dependency.

sffc · 2024-05-16T03:55:35Z

utils/zerovec/Cargo.toml

@@ -37,6 +38,7 @@ serde = { version = "1.0", default-features = false, features = ["alloc"], optio
 # and all 0.7 versions, but not further.
 yoke = { version = ">=0.6.0, <0.8.0", path = "../yoke", optional = true }
 twox-hash = { version = "1.6", default-features = false, optional = true }
+schemars = "0.8.16"


Yes, and please add it to the top-level Cargo.toml (ideally wait until #4844 is merged)

sffc · 2024-05-16T03:59:01Z

utils/zerovec/src/error.rs

@@ -33,6 +46,27 @@ impl fmt::Display for ZeroVecError {
        }
    }
 }
+impl JsonSchema for ZeroVecError {


Issue: I don't think you should need this impl. It should go away when you fix the ZeroSlice impl.

ashu26jha added 5 commits April 9, 2024 09:12

Implement JsonSchema for ZeroVec

9179c9b

Improved schema_id and tests

4e06ff3

Implement JsonSchema for VarZeroVec

6472c50

Implement for ZeroSlice & Errors

dc97369

error enums for schemars

395401f

ashu26jha requested review from Manishearth, sffc and a team as code owners April 10, 2024 16:03

ashu26jha commented Apr 10, 2024

View reviewed changes

Manishearth reviewed Apr 10, 2024

View reviewed changes

sffc reviewed Apr 12, 2024

View reviewed changes

Merge branch 'unicode-org:main' into schemars-support

5350349

sffc self-requested a review May 2, 2024 17:15

sffc requested changes May 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `schemars` to `ZeroVec` #4792

add `schemars` to `ZeroVec` #4792

ashu26jha commented Apr 10, 2024 •

edited

ashu26jha Apr 10, 2024 •

edited

Manishearth Apr 10, 2024

sffc May 16, 2024

Manishearth Apr 10, 2024

Manishearth Apr 10, 2024

sffc Apr 12, 2024

ashu26jha Apr 12, 2024 •

edited

sffc left a comment

sffc Apr 12, 2024

ashu26jha Apr 12, 2024

sffc Apr 12, 2024

sffc Apr 12, 2024

sffc Apr 12, 2024

sffc commented Apr 12, 2024

sffc commented Apr 12, 2024

ashu26jha commented Apr 12, 2024 •

edited

ashu26jha commented Apr 13, 2024 •

edited

ashu26jha commented Apr 16, 2024

sffc left a comment

sffc May 16, 2024

sffc May 16, 2024

sffc May 16, 2024

add schemars to ZeroVec #4792

Are you sure you want to change the base?

add schemars to ZeroVec #4792

Conversation

ashu26jha commented Apr 10, 2024 • edited

ashu26jha Apr 10, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashu26jha Apr 12, 2024 • edited

Choose a reason for hiding this comment

sffc left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Apr 12, 2024

sffc commented Apr 12, 2024

ashu26jha commented Apr 12, 2024 • edited

ashu26jha commented Apr 13, 2024 • edited

ashu26jha commented Apr 16, 2024

sffc left a comment

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

add `schemars` to `ZeroVec` #4792

add `schemars` to `ZeroVec` #4792

ashu26jha commented Apr 10, 2024 •

edited

ashu26jha Apr 10, 2024 •

edited

ashu26jha Apr 12, 2024 •

edited

ashu26jha commented Apr 12, 2024 •

edited

ashu26jha commented Apr 13, 2024 •

edited