Add support for binary custom metadata fields #102

per-gron · 2018-12-05T14:40:23Z

Adds support for base64 encoded "-bin" gRPC custom metadata.

[EDIT] I thought this would be enough to solve #79 but it's not: With this PR you can write custom metadata but not yet read it in a sane way, and it's not possible at all to differentiate between initial and trailing metadata in a response (not sure it even stores both).

Necessary for #27. See discussion at #90. This is a follow-up of #92.

Things to look for when reviewing:

How do you like what the user-facing API ends up like?
The ValueEncoding trait is afaik not importable from outside of this crate. Is that enough to prevent others from making implementations of it? It's not supposed to be possible.

Ping @carllerche here is a more or less fully fleshed out PR of what we discussed before.

What this commit does is to introduce _bin APIs for dealing with binary metadata keys, and distinct Binary vs Ascii MetadataKey types. It doesn't actually do any encoding or enforce any invariants yet.

…mespaces

It is just unhelpful for the user of MetadataMap that they were separate.

With the split of binary vs ascii keys I can't think of a reasonable way to implement this that doesn't have an API that is so complicated that it's not worth using for the very small performance benefits it provides. It's still possible to go down to the lower HeaderMap level and use drain from there.

That way it's more natural that both MetadataValue and MetadataKey use them.

This is a first step towards making the code automatically base64 encode and decode the binary values in MetadataMaps.

… Binary fields

They should be constructed from byte arrays, not strings.

The "name" name is an outdated reference to HeaderName.

Converting to Binary is possible but I can't think of a use case. Let's add it if it turns out it's useful. MVP etc.

I can't think of a reasonable use case where this shortcut helps meaningfully. Instead of implementing the necessary base64 encoding here I'll just remove it.

… way This is a step towards making this method do the right thing for MetadataValue<Binary>.

It seems unnecessary to provide this helper for binary values.

…e for Ascii It's not possible to return a slice to a the decoded base64 data for Binary values.

Turns out it is useful after all, otherwise there is no way to construct binary metadata values without copying.

seanmonstar

I think the plan here is pretty sound!

src/metadata_value.rs

src/metadata_encoding.rs

…nums

per-gron · 2018-12-10T11:28:33Z

I have addressed the review comments, except for preventing construction of the error types outside of the crate. I might be missing something but still think the potential future benefit of being able to add non-Default, non-Option fields to those error types (which I expect will remain empty structs forever) is much smaller than the current benefit of not requiring every single client with unit tests to wrap every call to this library in order to be able to test error conditions.

For fixing the build.

seanmonstar

🎉

per-gron · 2018-12-12T16:17:49Z

@carllerche do you also want to review this PR before I merge? I'll probably merge this tomorrow unless I get more comments.

carllerche · 2018-12-12T23:22:54Z

I'm back now! I will try to take a look in the next 24 hours. I'm still catching up on email :)

per-gron · 2018-12-13T11:56:34Z

@carllerche Aha, I see! No worries, I'm in no particular rush with this really. I'm off to vacation myself next week and will likely not spend much time on this until January.

carllerche

I love it! Thanks so much for this PR (and sorry for the very long 24 hours).

I like the API strategy you took. I think it is probably the best we can do.

I left some questions inline, but they are not related to this PR, so not worth blocking the merge.

carllerche · 2019-01-15T22:53:44Z

src/metadata_map.rs

-unsafe impl<'a> Sync for Drain<'a> {}
-unsafe impl<'a> Send for Drain<'a> {}
+unsafe impl<'a, VE: ValueEncoding> Sync for ValueDrain<'a, VE> {}
+unsafe impl<'a, VE: ValueEncoding> Send for ValueDrain<'a, VE> {}


Are these two unsafe impls required? The struct doesn't contain any unsafe types (the version in http did require it as it had raw pointers).

carllerche · 2019-01-15T22:55:40Z

src/metadata_map.rs

-unsafe impl<'a> Sync for ValueIterMut<'a> {}
-unsafe impl<'a> Send for ValueIterMut<'a> {}
+unsafe impl<'a, VE: ValueEncoding> Sync for ValueIterMut<'a, VE> {}
+unsafe impl<'a, VE: ValueEncoding> Send for ValueIterMut<'a, VE> {}


Same, I don't think these unsafe impls are needed?

per-gron added 30 commits December 5, 2018 11:32

Initial work towards supporting -bin fields in MetadataMap

7749629

What this commit does is to introduce _bin APIs for dealing with binary metadata keys, and distinct Binary vs Ascii MetadataKey types. It doesn't actually do any encoding or enforce any invariants yet.

Work towards placing Binary and Ascii keys in effectively separate na…

7022640

…mespaces

Merge contains_key_bin into contains_key

13efadd

It is just unhelpful for the user of MetadataMap that they were separate.

Update doc comments about binary vs ascii stuff

2ee8cd6

Move Ascii, Binary and ValueEncoding to its own file

aa38eff

That way it's more natural that both MetadataValue and MetadataKey use them.

Parameterize MetadataValue over ValueEncoding

e8e02a1

This is a first step towards making the code automatically base64 encode and decode the binary values in MetadataMaps.

Rename methods to make it obvious that they are unsafe

17d3b2c

Make MetadataMap::iter do the right thing wrt Binary fields

62c2754

Make MetadataMap::keys and MetadataMap::values do the right thing wrt…

919a383

… Binary fields

Update MetadataMap docs regarding -bin entries

9ef4c60

Test that getters are not panic prone with invalid key strings

cd32620

Document and test panic conditions better

f4b6a7c

Make MetadataMap::iter_mut do the right thing wrt Binary fields

9df7fb5

Make MetadataMap::values_mut do the right thing wrt Binary fields

79e0341

Flesh out the ValueEncoding trait a bit

6d86dd0

Don't have from_static, from_str, from_name for MetadataValue<Binary>

389a9a7

They should be constructed from byte arrays, not strings.

Rename MetadataValue::from_name to from_key

0aeb1be

The "name" name is an outdated reference to HeaderName.

Only allow conversion from MetadataKey to MetadataValue<Ascii>

5a9e33e

Converting to Binary is possible but I can't think of a use case. Let's add it if it turns out it's useful. MVP etc.

Re-add previously broken test

fd77846

Remove integer to MetadataValue<Binary> conversions

391632b

I can't think of a reasonable use case where this shortcut helps meaningfully. Instead of implementing the necessary base64 encoding here I'll just remove it.

Make never failing converter from byte slice to MetadataValue<Binary>

f695e72

Refactor MetadataValue::from_shared to use MetadataEncoding in a sane…

74284a1

… way This is a step towards making this method do the right thing for MetadataValue<Binary>.

Only allow MetadataValue::to_str on Ascii values

75b6e97

It seems unnecessary to provide this helper for binary values.

Define MetadataValue::to_bytes method and make as_bytes only availabl…

e2aa821

…e for Ascii It's not possible to return a slice to a the decoded base64 data for Binary values.

Add MetadataValue::as_encoded_bytes

87e22f6

Properly implement MetadataEncoding::is_empty

d7b230f

Actually base64 encode binary metadata values

64040c9

Re-add MetadataValue::<Binary>::from_static and add tests

54d7029

Turns out it is useful after all, otherwise there is no way to construct binary metadata values without copying.

Remove unnecessary TODO

223a827

per-gron requested a review from carllerche December 5, 2018 14:40

per-gron changed the title ~~Request metadata bin~~ Support for binary custom metadata fields Dec 5, 2018

per-gron changed the title ~~Support for binary custom metadata fields~~ Add support for binary custom metadata fields Dec 5, 2018

seanmonstar suggested changes Dec 5, 2018

View reviewed changes

Make ValueEncoding a "sealed" trait and make Ascii and Binary empty e…

feadd35

…nums

Make constructors Metadata related error types private to the crate

e2b29cd

per-gron mentioned this pull request Dec 11, 2018

require http 0.1.14, change MakeService to tower_util import #105

Merged

Merge remote-tracking branch 'origin/master' into request-metadata-bin

07d73e8

For fixing the build.

seanmonstar approved these changes Dec 11, 2018

View reviewed changes

Merge branch 'master' into request-metadata-bin

4b79a10

carllerche approved these changes Jan 15, 2019

View reviewed changes

carllerche merged commit 01defa8 into tower-rs:master Jan 15, 2019

vilunov mentioned this pull request Apr 11, 2019

Added custom_metadata interop test #153

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for binary custom metadata fields #102

Add support for binary custom metadata fields #102

per-gron commented Dec 5, 2018 •

edited

seanmonstar left a comment

per-gron commented Dec 10, 2018

seanmonstar left a comment

per-gron commented Dec 12, 2018

carllerche commented Dec 12, 2018

per-gron commented Dec 13, 2018

carllerche left a comment

carllerche Jan 15, 2019

carllerche Jan 15, 2019

Add support for binary custom metadata fields #102

Add support for binary custom metadata fields #102

Conversation

per-gron commented Dec 5, 2018 • edited

seanmonstar left a comment

Choose a reason for hiding this comment

per-gron commented Dec 10, 2018

seanmonstar left a comment

Choose a reason for hiding this comment

per-gron commented Dec 12, 2018

carllerche commented Dec 12, 2018

per-gron commented Dec 13, 2018

carllerche left a comment

Choose a reason for hiding this comment

carllerche Jan 15, 2019

Choose a reason for hiding this comment

carllerche Jan 15, 2019

Choose a reason for hiding this comment

per-gron commented Dec 5, 2018 •

edited