[Feature] Basic external ref support #31

lloydmeta · 2020-06-19T14:34:16Z

Closes #3

tl;dr Add basic support for "external" $ref fields that point to files.

When expanding fields, if field's ref is pointing to a .json file, resolve it, and merge all the types it references into the current parent Expander.

The changes in this PR do this by:

Adding field to Expander that holds the directory of the file from which its Schema is derived; this is re-used for reading relative referenced files.
When expanding a field that references an external file, create a child Expander to expand the referenced Schema and merging its data (types and further referenced Schemas) into the current one. Do this recursively.
Maybe using a tad too many clone()s...

To be honest, I need this for another project I plan on working on, so mostly hacked around until the tests passed without paying too much attention to efficiency. Here to learn so suggestions are welcome :)

Things not handled thus far:

Circular dependencies (compiler stack overflows)
Namespacing of types (model name clashes just fail the compile)

When expanding fields, if field's ref is pointing to a `.json` file, resolve it, and merge all the types it references into the current parent Expander. The changes here do this by: * Adding field to Expander that holds the directory of the file from which its Schema is derived; this is re-used for reading relative referenced files. * When expanding a field that references an external file, create a child Expander to expand the referenced Schema and merging its data (types and further referenced Schemas) into the current one. Do this recursively. * Maybe using a tad too many clone()s... Signed-off-by: lloydmeta <[email protected]>

Signed-off-by: lloydmeta <[email protected]>

Marwes

The cloning is going to be very expensive for large structures and could cause the code to run in exponetial time.

A relatively easy solution would be to add the ability to hook into the generation and allow it to replace the type being generated for any given field. Then override the type of any fields holding a Schema and make it hold an Arc<Schema> which would give us cheap clones.

schemafy_lib/src/lib.rs

Marwes · 2020-06-19T21:08:42Z

schemafy_lib/src/lib.rs

+        }
+    }
+
+    fn to_absolute_path(&self, s: &PathBuf) -> PathBuf {


Suggested change

fn to_absolute_path(&self, s: &PathBuf) -> PathBuf {

fn to_absolute_path(&self, s: &Path) -> PathBuf {

schemafy_lib/src/lib.rs

Marwes · 2020-06-19T21:12:53Z

schemafy_lib/src/lib.rs

@@ -571,7 +689,23 @@ impl<'r> Expander<'r> {
            None => self.expand_definitions(schema),
        }

-        let types = self.types.iter().map(|t| &t.1);
+        // Dedupe because we may have visited the same file multiple times when de-referencing
+        // external files


Shouldn't this be avoided altogether? If we have already visited a file it should be cached and not visited again

Good eye. I was going to go back and revisit this but forgot.

Had to do some refactoring/bug-fixing to get this fixed, but the short version is that while we don't visit the same file twice, when we kick off a child Expander for a new file, we shove all the types and Schemas from the parent Expander to it (to prevent unnecessary resolving.

when we merge the resolved types from the child Expander back into the parent Expander, we'd also get the same types that came from the Parent originally. I've updated types to be a HashMap to get around this and added a sort by type name before generating the AST to keep things deterministic (see
7976226)

lloydmeta · 2020-06-20T00:35:15Z

A relatively easy solution would be to add the ability to hook into the generation and allow it to replace the type being generated for any given field. Then override the type of any fields holding a Schema and make it hold an Arc which would give us cheap clones.

Can you give a concrete example of how to do what you’re suggesting? You say easy but it doesn’t sounds trivial given the bootstrapping of the structs.

I could just wrap all the Schemas in Expander in Arc (or even Rc since we’re not threaded?) quite easily though I think and get cheap clones.

Fewer PathBufs, more Paths Signed-off-by: lloydmeta <[email protected]>

lloydmeta · 2020-06-20T06:15:05Z

I gave the reference-counted Schema idea a shot: it turns out we also need to at least RefCell the Schema because merge_all_of needs a mutable Schema

schemafy/schemafy_lib/src/lib.rs

Lines 140 to 165 in 81ba451

    
           fn merge_all_of(result: &mut Schema, r: &Schema) { 
        
               use std::collections::btree_map::Entry; 
        
               for (k, v) in &r.properties { 
        
                   match result.properties.entry(k.clone()) { 
        
                       Entry::Vacant(entry) => { 
        
                           entry.insert(v.clone()); 
        
                       } 
        
                       Entry::Occupied(mut entry) => merge_all_of(entry.get_mut(), v), 
        
                   } 
        
               } 
        
               if let Some(ref ref_) = r.ref_ { 
        
                   result.ref_ = Some(ref_.clone()); 
        
               } 
        
               if let Some(ref description) = r.description { 
        
                   result.description = Some(description.clone()); 
        
               } 
        
               merge_option(&mut result.required, &r.required, |required, r_required| { 
        
                   required.extend(r_required.iter().cloned()); 
        
               }); 
        
               result.type_.retain(|e| r.type_.contains(e)); 
        
           }

called from

schemafy/schemafy_lib/src/lib.rs

Line 323 in 81ba451

merge_all_of(result.to_mut(), &self.schema(def));

Not sure at this point if I'm getting further and further away from what you're envisioning, so it's the last commit and can easily be reverted if you just want to drive bit that yourself.

lloydmeta · 2020-06-20T14:07:48Z

I opened another PR that contains a commit (14aef62) that might be closer to what you envisioned wrt adding a trigger to wrap Schema in a reference counting type, please take a look.

Use canonical paths to ensure we don't parse paths twice even when the references are relative. Signed-off-by: lloydmeta <[email protected]>

Signed-off-by: lloydmeta <[email protected]>

lloydmeta · 2020-06-27T07:13:52Z

@Marwes I'm closing this because I think #32 is the way forward with cheaper clones due to the wrapping.

lloydmeta added 2 commits June 19, 2020 23:24

* Add tests

6a36fa2

Signed-off-by: lloydmeta <[email protected]>

Marwes requested changes Jun 19, 2020

View reviewed changes

Review feedback

a847a20

Fewer PathBufs, more Paths Signed-off-by: lloydmeta <[email protected]>

lloydmeta force-pushed the feature/external-ref branch 2 times, most recently from de4e72e to 18a55f5 Compare June 20, 2020 05:56

lloydmeta mentioned this pull request Jun 20, 2020

[Feature] Basic external ref support redux #32

Closed

lloydmeta force-pushed the feature/external-ref branch from 18a55f5 to 30fc8c3 Compare June 20, 2020 14:31

lloydmeta added 2 commits June 20, 2020 23:34

Review feedback

54442d9

Use canonical paths to ensure we don't parse paths twice even when the references are relative. Signed-off-by: lloydmeta <[email protected]>

* With Rc and RefCell

b986b05

Signed-off-by: lloydmeta <[email protected]>

lloydmeta force-pushed the feature/external-ref branch from 30fc8c3 to b986b05 Compare June 20, 2020 14:34

lloydmeta closed this Jun 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Basic external ref support #31

[Feature] Basic external ref support #31

lloydmeta commented Jun 19, 2020

Marwes left a comment

Marwes Jun 19, 2020

Marwes Jun 19, 2020

lloydmeta Jun 20, 2020 •

edited

Loading

lloydmeta commented Jun 20, 2020

lloydmeta commented Jun 20, 2020

lloydmeta commented Jun 20, 2020 •

edited

Loading

lloydmeta commented Jun 27, 2020

	fn to_absolute_path(&self, s: &PathBuf) -> PathBuf {
	fn to_absolute_path(&self, s: &Path) -> PathBuf {

[Feature] Basic external ref support #31

[Feature] Basic external ref support #31

Conversation

lloydmeta commented Jun 19, 2020

Marwes left a comment

Choose a reason for hiding this comment

Marwes Jun 19, 2020

Choose a reason for hiding this comment

Marwes Jun 19, 2020

Choose a reason for hiding this comment

lloydmeta Jun 20, 2020 • edited Loading

Choose a reason for hiding this comment

lloydmeta commented Jun 20, 2020

lloydmeta commented Jun 20, 2020

lloydmeta commented Jun 20, 2020 • edited Loading

lloydmeta commented Jun 27, 2020

lloydmeta Jun 20, 2020 •

edited

Loading

lloydmeta commented Jun 20, 2020 •

edited

Loading