fix: use allocating deserializers when scratch space exceeded

[acfoltzer]

Issue #32 describes a class of errors where deserialize_str (and deserialize_bytes) have match expressions that fall through to an error when the Text (Bytes) lengths are longer than can fit in the scratch array.

This patch works around that limitation by delegating to the corresponding allocating methods deserialize_string (deserialize_byte_buf) in case the scratch space is too small.

This should address the issue even for types whose Deserialize visitor does not implement visit_string, since the default implementation of that method delegates back to visit_str once the fully-deserialized String is in hand.

This addition raises a question to me about the stance of the ciborium crate on no_std/alloc support. This workaround depends on being able to use String and Vec, which rules out no_std. But these allocating deserializers already exist in the code and don't appear to be guarded by cfg directives, so I don't believe this patch changes the level of support provided for these environments.

[acfoltzer]

@bstrie I rebased as you requested, but now there's a bunch of different clippy warnings, again not part of this change.

[bstrie]

@acfoltzer Thanks for the PR! (I just submitted a separate PR for the clippy stuff, let's not worry about that now.)

Can you add test cases for both deserialize_str and deserialize_bytes here?

This addition raises a question to me about the stance of the ciborium crate on no_std/alloc support.

This is a good question. I assume that the current behavior is deliberately intended to accommodate the no_std use case. Indeed, the crates.io tags include the "No standard library" tag, so that looks to be an intended use case. It does seem peculiar that there are allocating deserializers that aren't guarded by any cfg; there already exists a std feature flag in Cargo.toml so I presume that those were intended to be gated in such a way. Even if we don't know if the crate is currently insufficient for no_std, let's not take the risk of making it any worse, so you can gate the blocks that you're adding (and the aforementioned tests) on the std feature?

[bstrie]

Have you also seen https://github.com/enarx/ciborium/pull/43 ? It appears to be trying to solve the same problem (although it never got beyond the draft stage), and does take into account the no_std use case.

[acfoltzer]

Have you also seen #43 ? It appears to be trying to solve the same problem (although it never got beyond the draft stage), and does take into account the no_std use case.

I did see that, but given the current de-facto lack of no_std support and my limited time to address this, I opted for this more targeted fix instead. Maybe we could take the targeted approach for now to address the immediate bug, and then tackle the no_std/alloc-compatibility of the library in a future step?

Can you add test cases for both deserialize_str and deserialize_bytes here?

Unfortunately the tests in codec perform their roundtripping via Value which doesn't use those deserializers. Extending codec would involve more effort than I will be able to provide anytime soon (it is a busy season here) but I could probably throw together some one-off tests with a custom type if that works for you?

[bstrie]

@acfoltzer If no_std support is already broken then I'm fine accepting this PR without no_std guards, although I wouldn't want to actually perform a real release until no_std mode is fixed (although issuing a prerelease could be fine, if there's demand).

As far as testing, I'm happy to accept one-off tests, as long as they guard against regressions in the behavior being added here. I wouldn't want to close #32 without a regression test of some kind.

[bstrie]

I've filed #59 to track the no_std issue.

[acfoltzer]

@bstrie I've added some basic regression tests for the string and byte sequence cases.

Regarding a release, since the current release is not no_std compatible I don't think it'd be a regression to publish a patch release to fix this before addressing #59.

[bstrie]

@acfoltzer It appears that the tests fail in the case of cargo test --no-default-features, IOW when the std feature isn't enabled. Can you gate them?