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.

Sign up for GitHub

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

Jump to bottom

Add support for [u8; N] #28

Merged
merged 2 commits into from
Dec 27, 2023
Merged

Add support for [u8; N] #28

merged 2 commits into from
Dec 27, 2023

Conversation

sgued
Copy link
Contributor

@sgued sgued commented Mar 24, 2022 •
edited
Loading

Hi.

This PR adds support for constant length byte arrays [u8; N] and adds a ByteArray<const N: usize> wrapper type similar to BytesBuf. This would close #26.

Motivations

Fixed sized array are often used for cryptography and embedded systems. Having efficient support to serialize/deserialize keys would be great.

Parts of the PR requiring discussion

I'm not sure why ByteBuf and Bytes implements PartialEq and PartialOrd for types implementing AsRef<[u8]> and not Borrow<[u8]>. ByteArray implements PartialEq/Ord with types implementing Borrow<[u8; N]> so that it can be compared to itself and to [u8; N]

It might be worth considering adding a new_mut(&mut [u8]) -> Bytes function to Bytes so that the same (unsafe) code is reused for the BorrowMut<Bytes> implementations of ByteArray and ByteBuf.

Drawbacks

Due to the use of const generics, this would require raising the MSRV to 1.53 (which could be lowered to 1.51 by removing the IntoIterator implementation).

Edit: Since 0504fcb already bumped MSRV to 1.56, this drawback doesn't apply anymore.

@Xiretza
Copy link
Contributor

Xiretza commented May 27, 2022

I'd love to see this added, would it be possible for a maintainer to have a look at it?

Xiretza
Xiretza reviewed May 27, 2022
View reviewed changes
src/bytearray.rs Outdated Show resolved Hide resolved
@sgued sgued force-pushed the byte-array branch 2 times, most recently from e3a2120 to 846ccba Compare July 16, 2022 12:53
@kamulos
Copy link

kamulos commented Aug 8, 2022

I am also really interested in this. Any chance you could have a look @dtolnay ?

martin-g
martin-g reviewed Aug 30, 2022
View reviewed changes
src/bytearray.rs Outdated Show resolved Hide resolved
src/bytearray.rs Outdated Show resolved Hide resolved
martin-g
martin-g reviewed Aug 30, 2022
View reviewed changes
Cargo.toml Show resolved Hide resolved
@sgued sgued force-pushed the byte-array branch 2 times, most recently from 7b125bc to 1bdc428 Compare August 30, 2022 10:53
martin-g
martin-g reviewed Aug 30, 2022
View reviewed changes
Copy link

@martin-g martin-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

martin-g added a commit to apache/avro that referenced this pull request Oct 3, 2022
@martin-g
AVRO-3631: Add support for ser_de Value::Fixed
7b1cdd8 
It is based on serde-rs/bytes#28 which is not
yet merged.

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
@martin-g martin-g mentioned this pull request Oct 3, 2022
Closed
@elichai
Copy link

elichai commented Oct 19, 2022

Note that unlike serde serde_bytes doesn't have the impl Serialize for [T; 0] problem

Lucretiel
Lucretiel reviewed Oct 27, 2022
View reviewed changes
src/bytearray.rs Outdated Show resolved Hide resolved
Lucretiel
Lucretiel reviewed Oct 27, 2022
View reviewed changes
src/bytearray.rs Outdated Show resolved Hide resolved
Lucretiel
Lucretiel reviewed Nov 2, 2022
View reviewed changes
src/bytearray.rs Outdated Show resolved Hide resolved
src/bytearray.rs Outdated Show resolved Hide resolved
src/bytearray.rs Outdated Show resolved Hide resolved
Lucretiel
Lucretiel reviewed Nov 2, 2022
View reviewed changes
src/bytearray.rs Outdated Show resolved Hide resolved
@ryoqun ryoqun mentioned this pull request Dec 19, 2022
Merged
11 tasks
markfarnan added a commit to markfarnan/avro that referenced this pull request Dec 30, 2022
@markfarnan
Merged AVRO-3631 - Fix for serialization of FIXED [u8,16] - req for u…
5f593da 
…uid.

Squashed commit of the following:

commit 3a1fac3
Author: Martin Tzvetanov Grigorov <[email protected]>
Date:   Fri Oct 7 10:48:13 2022 +0300

    AVRO-3631: Use official serde_bytes crate

    Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

commit f7f0ec5
Author: Martin Tzvetanov Grigorov <[email protected]>
Date:   Thu Oct 6 14:01:19 2022 +0300

    AVRO-3631: Fix clippy issues

    Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

commit 0ee5b00
Author: Martin Tzvetanov Grigorov <[email protected]>
Date:   Thu Oct 6 13:38:10 2022 +0300

    AVRO-3631: Add more test cases

    Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

commit 70d6cf0
Author: Martin Tzvetanov Grigorov <[email protected]>
Date:   Thu Oct 6 12:05:08 2022 +0300

    AVRO-3631: Fix clippy and Rat issues

    Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

commit 7c9e938
Author: Martin Tzvetanov Grigorov <[email protected]>
Date:   Thu Oct 6 11:45:50 2022 +0300

    AVRO-3631: Add serde serialize_with functions

    Those should be used for hinting the serialization process how to serialize a byte array to Value::(Bytes|Fixed)

    Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

commit 3e3f125
Author: Martin Tzvetanov Grigorov <[email protected]>
Date:   Mon Oct 3 23:45:11 2022 +0300

    AVRO-3631: Use #[serde(with)] attribute to get rid of implementation detail ByteArray

    Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

commit 7b1cdd8
Author: Martin Tzvetanov Grigorov <[email protected]>
Date:   Mon Oct 3 14:52:24 2022 +0300

    AVRO-3631: Add support for ser_de Value::Fixed

    It is based on serde-rs/bytes#28 which is not
    yet merged.

    Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

commit 5c2d197
Author: Rik Heijdens <[email protected]>
Date:   Thu Sep 29 14:15:49 2022 +0200

    AVRO-3631: Reformat using rustfmt

commit a31fcfc
Author: Rik Heijdens <[email protected]>
Date:   Thu Sep 29 14:14:35 2022 +0200

    AVRO-3631: Add test for serializing fixed fields

    This test-case mainly demonstrates the issue reported in AVRO-3631. It
    is unclear to me whether we should actually expect the serializer to
    serialize to a Value::Fixed right away given that Schema information is
    not available at this stage.

commit 12ef14b
Author: Rik Heijdens <[email protected]>
Date:   Thu Sep 29 14:12:51 2022 +0200

    AVRO-3651: Add test to de.rs to illustrate issue with Fixed fields

commit d32d436
Author: Rik Heijdens <[email protected]>
Date:   Wed Sep 28 15:48:29 2022 +0200

    AVRO-3631: Add test-case to reproduce
@markfarnan
Copy link

This is blocking downstream feature in Avro/Rust for Fixed Types.
I've tested it, and it works fine for my use case with Avro.

Can this be merged soon please ?

@emchristiansen
Copy link

Haha, I created a pull request with almost exactly the same name, not realizing that this existed (I'll delete it now).
But, can we please get this merged in?

martin-g added a commit to apache/avro that referenced this pull request Feb 9, 2023
@martin-g
AVRO-3631: Add support for ser_de Value::Fixed
d674db9 
It is based on serde-rs/bytes#28 which is not
yet merged.

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
@sosthene-nitrokey sosthene-nitrokey mentioned this pull request Feb 22, 2023
Merged
@sosthene-nitrokey
Copy link
Contributor

Given the lack of activity in this PR we published a similar crate specifically for [u8; N] as serde-byte-array

@jonasbb
Copy link

jonasbb commented Feb 24, 2023

The serde_with::Bytes type supports const generic arrays in a bunch of ways.

martin-g added a commit to apache/avro that referenced this pull request Mar 13, 2023
@martin-g
AVRO-3631: Add support for ser_de Value::Fixed
e9b7853 
It is based on serde-rs/bytes#28 which is not
yet merged.

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
martin-g added a commit to apache/avro that referenced this pull request Mar 24, 2023
@martin-g
AVRO-3631: Add support for ser_de Value::Fixed
9ab5358 
It is based on serde-rs/bytes#28 which is not
yet merged.

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
@emchristiansen
Copy link

Does anyone know why this is taking so long?
I.e., should I wait for a fix or should I switch to a different crate?

@so-schen
Copy link

in case anyone looking for drop-in replacement with all bytes/slice support in one crate, can use this https://github.com/so-schen/serde-bytes-ng

dtolnay
dtolnay approved these changes Dec 27, 2023
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This looks great, especially with Lucretiel great suggestions.

@dtolnay dtolnay merged commit 5da9904 into serde-rs:master Dec 27, 2023
8 checks passed
@sgued sgued mentioned this pull request Dec 27, 2023
Open
@sosthene-nitrokey sosthene-nitrokey mentioned this pull request Dec 27, 2023
Open
martin-g added a commit to apache/avro that referenced this pull request Apr 9, 2024
@martin-g
AVRO-3631: Add support for ser_de Value::Fixed
c999938 
It is based on serde-rs/bytes#28 which is not
yet merged.

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
martin-g added a commit to apache/avro that referenced this pull request Jul 17, 2024
@martin-g
AVRO-3631: Add support for ser_de Value::Fixed
172fb85 
It is based on serde-rs/bytes#28 which is not
yet merged.

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xiretza Xiretza Xiretza left review comments

@Lucretiel Lucretiel Lucretiel left review comments

@martin-g martin-g martin-g left review comments

@dtolnay dtolnay dtolnay approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement (de-)serializing for [u8; N]