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

Support for repr attribute #282

Merged
merged 6 commits into from
Sep 15, 2022
Merged

Support for repr attribute #282

merged 6 commits into from
Sep 15, 2022

Conversation

Naymoll
Copy link
Contributor

@Naymoll Naymoll commented Sep 14, 2022

I tried to write a simple impl support of repr attribute. Right now there is no support for serde tags.

If we have something like this:

#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, ToSchema)]
#[repr(u8)]
pub enum OsrmVersion {
    Osrm4 = 0,
    Osrm5,
}

Macro generates the following output:

impl utoipa::ToSchema for OsrmVersion {
    fn schema() -> utoipa::openapi::schema::Schema {
        utoipa::openapi::ObjectBuilder::new()
            .schema_type(utoipa::openapi::SchemaType::Integer)
            .enum_values(Some([Self::Osrm4 as u8, Self::Osrm5 as u8]))
            .into()
    }
}

*.json file

"OsrmVersion": {
    "type": "integer",
    "enum": [
        "0",
        "1"
}

Related to: #271

@Naymoll
Copy link
Contributor Author

Naymoll commented Sep 14, 2022

Also, I haven't checked how schema attributes affect macro generation.

@juhaku
Copy link
Owner

juhaku commented Sep 14, 2022

This is a pretty good start 💪 but support for serde attributes would be beneficial, but the rename of enum at container level is no prio since throughout the lib there is no such support and implementing it needs more planning. And tests would be be useful. There are plenty in schema_derive_test.rs file 🙂

@Naymoll
Copy link
Contributor Author

Naymoll commented Sep 14, 2022

I think the tag and skip attributes need to be supported from serde. The others do not make sense. I will try to write an implementation. Tests as well.

@Naymoll
Copy link
Contributor Author

Naymoll commented Sep 14, 2022

I noticed one thing. Even if the enum type is marked as integer, the value itself is still a string. So when I make a request to the server, it sends this:

"configuration": {
    ...
    "osrm_version": "0",
    ...
}

Which is incorrect:

{"error":"Json deserialize error: invalid type: string \"0\", expected u8 at line 7 column 27"}

I've already changed enum_values, so you can pass whatever that impl ToString. But I think that it's wrong. And more changes are needed. Any advice?

@juhaku
Copy link
Owner

juhaku commented Sep 14, 2022

Keep in mind that the OpenAPI specification does not allow integer type of enums. https://swagger.io/specification/ If you browse to the spec and search for enum you find that it is always used as array of string types.

@Naymoll
Copy link
Contributor Author

Naymoll commented Sep 14, 2022

Didn't know that. Then all that's left is to write the tests.

@Naymoll Naymoll changed the title Draft: Support for repr attribute Support for repr attribute Sep 14, 2022
@Naymoll
Copy link
Contributor Author

Naymoll commented Sep 14, 2022

I think that's all for now.

@juhaku
Copy link
Owner

juhaku commented Sep 14, 2022

Keep in mind that the OpenAPI specification does not allow integer type of enums. https://swagger.io/specification/ If you browse to the spec and search for enum you find that it is always used as array of string types.

Sorry, I eat my words, I tested it in editor.swagger.io and seems like the enum can also be represented according the type of the field. e.g.

components:
  schemas:
    Order:
      type: object
      properties:
        status:
          type: integer
          description: Order Status
          enum:
            - 0
            - 1
            - 2

@juhaku
Copy link
Owner

juhaku commented Sep 14, 2022

I've already changed enum_values, so you can pass whatever that impl ToString. But I think that it's wrong. And more changes are needed. Any advice?

Actually to have it support dynamic enum values there is a need to change the enum_values representation within the Object type in utoipa crate.

pub struct Object {
// ...
    #[serde(rename = "enum", skip_serializing_if = "Option::is_none")]
    pub enum_values: Option<Vec<String>>,

The string needs to be changed to serde_json::Value see the example field. And then of course fix all the existing usages which currently expect it to be string.

Copy link
Owner

@juhaku juhaku left a comment

Choose a reason for hiding this comment

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

Overall this is okay there are 2 things still address here 🙂

  • Add this repr create support behind a repr feature flag, so people can enable it when they want.
  • Add docs about this new functionality to utoipa-gen/src/lib.rs and list the new feature also in README.md and in utoipa/src/lib.rs

You can generate the doc with, and then use browser to see the output.

cargo doc --workspace --no-deps --features utoipa/json,utoipa/actix_extras,utoipa/openapi_extensions

@Naymoll Naymoll requested a review from juhaku September 15, 2022 10:00
Copy link
Owner

@juhaku juhaku left a comment

Choose a reason for hiding this comment

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

🏆 pretty good, let's get it merged

@juhaku juhaku merged commit 503042f into juhaku:master Sep 15, 2022
@Naymoll Naymoll deleted the feature/Enum-repr branch September 15, 2022 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Released
Development

Successfully merging this pull request may close these issues.

2 participants