compose_yml icon indicating copy to clipboard operation
compose_yml copied to clipboard

`validate_file` is a no-op because `valico` isn't compatible with `serde` 1.0

Open emk opened this issue 8 years ago • 4 comments

The validate_file function in compose_yml is supposed to make sure that all the files we read and write correspond to an appropriate JSON schema. Unfortunately, this relies on valico, which appears to be currently unmaintained, and which only works with serde 0.8.

See https://github.com/rustless/valico/pull/27 for a PR that would fix this.

emk avatar Dec 15 '17 15:12 emk

@camjackson We can certainly just disable this warning.

The lack of valico means that we probably have worse error messages, and we don't validate inputs and outputs as well as we should.

The real fix is to get a version of valico with the fixes mentioned above merged (and ditto for a second crate used by valico). At this point, that probably means forking both libraries and releasing them to crates.io.

There's also a trick we could do by releasing a single shim library that re-exports the older serde APIs (And serde JSON?) under a new name.

Probably the first step would be to ping upstream more aggressively.

But in the meantime, we can turn this warning off or replace it by debug! if that helps. Thoughts?

emk avatar Dec 17 '17 10:12 emk

This is the nature of open source I guess!

As an end user, I think it would be best if Cage didn't log the extra information. It's quite noisy to have it there on every cage command, especially multiple times per command. But I also see the value in having something there in the code as a reminder that the validation function is a noop.

Would a debug! be silent when compiled into Cage? If so, that seems like a good idea to me. Or if debug! will still show up in the console, then maybe a comment would be better.

camjackson avatar Dec 17 '17 10:12 camjackson

A debug! would be silent unless logging were manually elevated. Or, actually, we could look at this line here, and add another rule for compose_yml::v2::validate, setting it to log::LogLevelFilter::Error.

On the other hand, this is actually a real compose_yml and cage regression (and not a particularly small regression!) and the best action would be to fix it ASAP.

So, choices:

  1. Convince upstream to merge https://github.com/rustless/valico/pull/27 and incorporate https://github.com/korczis/jsonway/commit/8a142a0799d9f7afdf5ec1957c1c6791b80d8013.
  2. less good Fork both libraries, rename them, and release them to crates.io. I'd prefer to avoid this. The above PRs have only been waiting for a month. (However, the underlying libraries haven't been updated in a year or so.)
  3. Build a shim library around serde_json which exports the old APIs under a different namespace, so that two different versions of serde_json can be used internally by compose_yml without conflict. Rust can actually handle this.

But the problem with (3) is that once we start releasing shim crates, why not just fork the crates we actually need and update them?

I can't find any other JSON Schema implementations in Rust at the moment. valico is the best game in town, AFAICT.

emk avatar Dec 17 '17 11:12 emk

That all makes sense. Maybe let's put this on hold for a short while, giving upstream developers a chance to respond. And if they don't have bandwidth then we can consider the options further.

camjackson avatar Dec 17 '17 11:12 camjackson