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.

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

Poem converts cases differently than serde #362

Closed
banool opened this issue Aug 9, 2022 · 8 comments
Closed

Poem converts cases differently than serde #362

banool opened this issue Aug 9, 2022 · 8 comments
Labels
bug Something isn't working

Comments

@banool
Copy link
Contributor

banool commented Aug 9, 2022

Code demonstrating the issue:

use serde::{Deserialize, Serialize};
use poem_openapi::{Object, Union};
use poem_openapi::types::ToJSON;

#[derive(Clone, Debug, PartialEq, Serialize, Deserialize, Object)]
pub struct Ed25519Signature {}

#[derive(Clone, Debug, PartialEq, Serialize, Deserialize, Object)]
pub struct MultiEd25519Signature {}

#[derive(Clone, Debug, PartialEq, Serialize, Deserialize, Object)]
pub struct MultiAgentSignature {}

#[derive(Clone, Debug, PartialEq, Serialize, Deserialize, Union)]
#[serde(tag = "type", rename_all = "snake_case")]
#[oai(one_of, discriminator_name = "type", rename_all = "snake_case")]
pub enum TransactionSignature {
    Ed25519Signature(Ed25519Signature),
    MultiEd25519Signature(MultiEd25519Signature),
    MultiAgentSignature(MultiAgentSignature),
}

fn main() {
    let test = TransactionSignature::Ed25519Signature(Ed25519Signature{});
    println!("test: {}", serde_json::to_string(&test).unwrap());
    println!("test: {}", test.to_json().unwrap());
}

Output:

test: {"type":"ed25519_signature"}
test: {"type":"ed_25519_signature"}

I feel we should strive to make the behavior match here.

@banool
Copy link
Contributor Author

banool commented Aug 9, 2022

Upon diving deeper, I see Poem uses inflector while serde uses its own thing: https://github.com/serde-rs/serde/blob/master/serde_derive/src/internals/case.rs.

@banool
Copy link
Contributor Author

banool commented Aug 9, 2022

I don't think I have the context necessary to make a change here, do you have any ideas @sunli829? I figure the best way would be to leverage serde more directly (perhaps optionally, only if the user wants) instead of implementing rename_all separately. This kind of thing would enable #329 too.

@banool banool changed the title API serializes certain fields as snake case differently than serde Poem converts cases differently than serde Aug 10, 2022
@banool
Copy link
Contributor Author

banool commented Aug 10, 2022

Seems like the serde stuff is accessible: https://crates.io/crates/serde_derive_internals. I'll make a PR if you think the idea to switch over makes sense @sunli829.

@sunli829
Copy link
Collaborator

Can't use serde directly, because can't fully support the serde annotation, I currently use the Inflector crate for case conversion, I will try another one.

@banool
Copy link
Contributor Author

banool commented Aug 10, 2022

Perhaps relying on serde_derive_internals might be the best option? Let me know if you're working on this, if not I can try implement it myself :)

@sunli829
Copy link
Collaborator

I'm working on this.

@sunli829
Copy link
Collaborator

sunli829 commented Aug 10, 2022

I've push changes in master branch, please help test it.

@banool
Copy link
Contributor Author

banool commented Aug 10, 2022

Works perfectly, thanks a lot! Using it in aptos-labs/aptos-core#2764 to great effect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants