Actix plugin: add an empty impl for actix-web ReqData

[dsferruzza]

Hi!

I tried to fix #300 and it seems to work!
(I tested it locally on a project that uses paperclip.)

Could someone review this, please?

[tiagolobocastro]

Seems ok to me. Could you please add a test case?

[dsferruzza]

@tiagolobocastro Would something like ca943e6 do the trick?

[tiagolobocastro]

@tiagolobocastro Would something like ca943e6 do the trick?

Yes indeed 👍

[tiagolobocastro]

@dsferruzza, they don't look like they're caused by your changes, but do you mind fixing the build failures?

[dsferruzza]

@tiagolobocastro There are 2 "easy-to-fix" issues, but I'd like your opinion:

1. cargo check reports that ValidationError::InvalidRefURI should be ValidationError::InvalidRefUri. I'm fine with both but I think it might be cool to avoid breaking backward compatibility and just ignore this warning with #[allow(clippy::upper_case_acronyms)]
2. Some snapshot testing breaks because quotes appeared to be falsely escaped in a generated YAML file before (see following screenshot). I guess I can just accept the new snapshot and call it a day

Shall I proceed like I said?

[tiagolobocastro]

@dsferruzza that sounds good

[dsferruzza]

@tiagolobocastro There is something weird about item number 2: my "fix" on test-nightly actually works but broke test. So there is a change of behavior (or a bug) from stable to nightly but I have no idea where... And I'm not sure what is the correct expected test output here. Any idea?

[tiagolobocastro]

@tiagolobocastro There is something weird about item number 2: my "fix" on test-nightly actually works but broke test. So there is a change of behavior (or a bug) from stable to nightly but I have no idea where... And I'm not sure what is the correct expected test output here. Any idea?

That's very odd, sadly doesn't seem to happen on my box :/ Could you undo the change, and print the contents of data in assert_file to find out if it's the the read_to_string that's escaping the quotes or if somehow it's the insta crate?

[dsferruzza]

I tracked down the bug to the codegen module: the write_clap_yaml function behave differently when compiled using stable vs. nightly. The produced tests/test_k8s/cli/app.yaml is different!

• the faulty line is here: https://github.com/wafflespeanut/paperclip/blob/master/src/v2/codegen/impls.rs#L148
• Debug is used to escape and output some strings to YAML properties, but there is a known issue on the escaping part → String Debug implementation prints escaped single quotes rust-lang/rust#83046
• the issue was fixed in Rust master/nightly and seems to be scheduled for release in Rust 1.53.0

At this point, I cannot think of a way to fix both tests (stable and nightly) without doing a dirty hack (re-implementing escaping, doing \' → ' replacements, …).

1. Any idea?
2. By the way, if I'm correct this might mean that codegen is currently broken on stable: if you have single quotes in a description it might be falsely escaped in the "Clap YAML" output

[tiagolobocastro]

Good catch!
This is unfortunate then... would using ToString rather than {:?} be possible? Though even if possible maybe that would be too much work for this item, so I think doing the "dirty hack" for the tests is enough for now.

It'd be great if you can confirm that codegen is broken on stable (https://paperclip.waffles.space/cli.html) and raise a ticket for it

[dsferruzza]

Using Display/ToString instead of Debug does not do the trick because we actually need the string to be escaped and quoted like Debug does (because the YAML file seems to be built by appending strings 😬). I will try to land something that works for the tests…

Codegen seems to work if I put single quotes in my definition file (pet-v2-single-quotes.yaml.txt). The part that fails in the test is related to the write_clap_yaml function; I do not know what it is supposed to achieve exactly nor if it is actually executed when using the CLI…

I believe it gets called at some point when this code is executed from the faulty test (test_clap_yaml):

static K8S_SCHEMA: Lazy<ResolvableApi<K8sSchema>> = Lazy::new(|| {
    let fd = File::open(ROOT.join("tests/k8s-v1.16.0-alpha.0-openapi-v2.json")).expect("file?");
    let raw: ResolvableApi<K8sSchema> = v2::from_reader(fd).expect("deserializing spec");
    raw.resolve().expect("resolution")
});
// [...]
static CODEGEN_K8S_CLI: Lazy<()> = Lazy::new(|| {
    let mut state = EmitterState::default();
    state.working_dir = (&*ROOT).into();
    state.working_dir.push("tests/test_k8s/cli");
    let mut meta = CrateMeta::default();
    assert_eq!(meta.mode, EmitMode::Module);
    meta.name = Some("test-k8s-cli".into());
    meta.version = Some("0.0.0".into());
    meta.authors = Some(vec!["Me <[email protected]>".into()]);
    meta.mode = EmitMode::App;
    state.set_meta(meta);

    let emitter = DefaultEmitter::from(state);
    emitter.generate(&K8S_SCHEMA).expect("codegen");
});

[dsferruzza]

@tiagolobocastro I guess the build is fixed! Does it seem acceptable to you?

[tiagolobocastro]

@tiagolobocastro I guess the build is fixed! Does it seem acceptable to you?

Nice one, seems reasonable to me. Would you mind rebasing your 4 "fix ***" commits into a single one?

[dsferruzza]

Done!