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

Allow Schema Value to be Owned #145

Closed
kayyagari opened this issue Nov 16, 2020 · 12 comments
Closed

Allow Schema Value to be Owned #145

kayyagari opened this issue Nov 16, 2020 · 12 comments

Comments

@kayyagari
Copy link

In the current form it is impossible to retain a non-static reference to JSONSchema until the end of the program's execution. The below block demonstrates the problem.

#[actix_web::main]
async fn main() -> std::io::Result<()> {
    let schema_file = fs::File::open("config/schema.json").unwrap();

    let schema: Value = serde_json::from_reader(schema_file).unwrap();
    let validator = JSONSchema::compile(&schema, None).unwrap();

    let ad = AppData{
        validator: Arc::new(validator)
    };

    HttpServer::new(move ||{
        App::new()
            .data(ad.clone())
            .service(echo)
    })
    .bind("localhost:9070")?
    .run()
    .await
}

#[derive(Clone)]
pub struct AppData<'a> {
    pub validator: Arc<JSONSchema<'a>>
}

The above code results in the below compiler error:

error[E0597]: `schema` does not live long enough
  --> src/main.rs:15:41
   |
15 |     let validator = JSONSchema::compile(&schema, None).unwrap();
   |                     --------------------^^^^^^^-------
   |                     |                   |
   |                     |                   borrowed value does not live long enough
   |                     argument requires that `schema` is borrowed for `'static`
...
31 | }
   | - `schema` dropped here while still borrowed

One(or the only?) way to get this fixed is to let JSONSchema own the Value containing schema.

kayyagari added a commit to kayyagari/barn that referenced this issue Nov 22, 2020
@kyle-mccarthy
Copy link

@kayyagari I know that this doesn't address making the schema value owned, but your example is probably a good use case for Box::leak. In this case leaking the memory doesn't matter since you want the value to exist for the duration of the program.

let schema: Value = serde_json::from_reader(schema_file).unwrap();
let schema_boxed: &'static Value = Box::leak(Box::new(schema));
let validator = JSONSchema::compile(&schema_boxed).unwrap();

@lichr
Copy link

lichr commented Mar 7, 2021

I have encountered same issue, tried to workaround it by lying to the compiler about the lifetime of the input json like this:

/// IMPORTANT: this struct must not be moved
/// this is a self referential struct: JSONSchema holds a reference to doc
pub struct Schema<'a> {
    // step1: we need to move doc here, so that it's address is fixed
    pub doc: Value,
    // step2: we create schema with self.doc, and move it here
    pub compiled: Option<JSONSchema<'a>>,
}

impl<'a> Schema<'a> {
    fn compile(&mut self, options: &CompilationOptions) {
        // IMPORTANT: here we lie about doc's lifetime
        let doc: &'static Value = unsafe { std::mem::transmute(&self.doc) };
        let compiled = options.compile(doc).unwrap();
        self.compiled = Some(compiled);
    }
}

What I was doing is to load many schemas and put them into a hashmap:

{
    schemas: HashMap<String, Box<Schema<'a>>>,
}

// at some point:
            let mut schema = Box::new(Schema {
                compiled: None,
                doc: doc.clone(),
            });
            schema.compile(&self.options);
            self.schemas.insert(name.to_string(), schema);

I think one possible way to improve this is to let the compile method 'consume' the json Value so that the resulted JSONSchema contains the input data instead of holding a reference, this could make this use-case much simpler.

@johnsom
Copy link

johnsom commented Apr 24, 2021

I also hit this issue and banged my head against the wall on it. I'm learning Rust and thought I was doing something stupid.
I agree, having compile consume the Value would make using this more obvious.
Thanks for the crate!

@elbaro
Copy link

elbaro commented Jun 4, 2021

A common use case is to compile a schema once and reuse it for validation multiple times.
In this case, we don't need to compile fast by avoiding the copy.
In fact, I guess the owned version will validate faster due to memory locality.

@luke-biel
Copy link

I would love this to be a feature. It's a mess when trying to create a service that validates multiple object types with caching built in.

@robot-head
Copy link

Also running into this exact issue building exactly this:

I would love this to be a feature. It's a mess when trying to create a service that validates multiple object types with caching built in.

Is the official answer to do a Box::leak? I want to put this inside a OnceCell that pre-caches the schemas at service startup

@alexjg
Copy link
Contributor

alexjg commented Jul 19, 2021

This seems like it would also be useful for ValidationErrors. Currently implementing any kind of type which wraps a validation error requires holding on to the input reference somewhere, this can be irritating if you're doing something like accumulating errors over many instances which are each transient.

@Stranger6667
Copy link
Owner

Stranger6667 commented Jul 19, 2021

Folks, I completely agree with the mentioned points and would like to have it changed this way.

Maybe I overestimated the impact (compilation usually happens much less often than validation), and it will be alright to clone the input schema into resolver under the hood. If anybody knows any good way to have it more compact, I'd be happy to learn about it.

Also, I am sorry that this design flaw got you blocked from using the library in the mentioned use cases for such a long time.

At the moment, I don't have much bandwidth to work on this, but I will happily review a PR that brings this change.

@alexjg
Copy link
Contributor

alexjg commented Jul 19, 2021

I can take this on. My main question is do we care about API breakage?

@Stranger6667
Copy link
Owner

@alexjg

Awesome! I think that with this kind of change, the breakage is inevitable and is nicely motivated by the comments above. However, it would be nice to reduce the breakage surface if we can, but it should not block us from this kind of usability improvement.

@Stranger6667
Copy link
Owner

Solved in #255 by @alexjg ! :) Thank you very much for implementing this :)

@Stranger6667
Copy link
Owner

Released in 0.12.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants