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

Actix-web: Support enum variants with named fields #97

Open
Ploppz opened this issue Sep 5, 2019 · 18 comments · May be fixed by #389
Open

Actix-web: Support enum variants with named fields #97

Ploppz opened this issue Sep 5, 2019 · 18 comments · May be fixed by #389
Labels
enhancement New feature or request epic A fair amount of work is involved research Some investigation required prior to making changes.

Comments

@Ploppz
Copy link

Ploppz commented Sep 5, 2019

As discussed here: oneOf, anyOf, allOf.

Filing this issue in order to track when this will be supported.

@wafflespeanut
Copy link
Collaborator

Thanks! I still have to open a great deal of issues. Slowly progressing 😄

@wafflespeanut wafflespeanut added epic A fair amount of work is involved research Some investigation required prior to making changes. labels Sep 5, 2019
@wafflespeanut wafflespeanut added the enhancement New feature or request label Sep 5, 2019
@wafflespeanut wafflespeanut changed the title Support enum variants with named fields Actix-web: Support enum variants with named fields Sep 7, 2019
@wafflespeanut wafflespeanut changed the title Actix-web: Support enum variants with named fields Actix-web: Support enums Nov 15, 2019
@wafflespeanut wafflespeanut changed the title Actix-web: Support enums Actix-web: Support enum variants with named fields Nov 15, 2019
@wafflespeanut
Copy link
Collaborator

I thought I could add some of v3 features (including this) to v2 through extensions, but 0.4.0 has already taken an awful lot of time. I'm now moving this to 0.5.0.

@oli-cosmian
Copy link
Contributor

I'd like to work on this, but I don't understand how we could add this to v2 so that e.g. swagger-ui could process it just like it would in v3. Do you have some hints on how to get into this?

@wafflespeanut
Copy link
Collaborator

I'm really happy to hear that you want to work on this, but I only have one solution as of now, and I think it's gonna be a lot of work.

The problem is that we can't add this to v2 directly. My plan is to make use of extensions in v2 (fields of pattern ^x-* are allowed throughout the spec) to support features introduced in v3. Paperclip (codegen and plugins) will understand these extensions. This way, we can still do everything in v2 and convert to v3 whenever we want (including the time when we host the spec in the actix-web app instance). Swagger UI will process the spec emitted in v3. The work involved is more on the v2-v3 conversion, which is why I moved this to 0.5.0.

@oli-cosmian
Copy link
Contributor

oli-cosmian commented Apr 2, 2020

Ah, so as a first step we need v2 to v3 conversion in general? I couldn't find an issue for v3 yet (except the v3 to v2 conversion for codegen, which seems mostly orthogonal?)

@wafflespeanut
Copy link
Collaborator

Yes, there wasn't an issue until now. #177 😅

@wafflespeanut
Copy link
Collaborator

wafflespeanut commented Apr 11, 2020

Even though OpenAPI doesn't have proper enum support (it only supports simple enums with known values), swagger does support an array of enums (through items field). For example:

{
  "definitions": {
    "MyEnum": {
      "type": "array",
      "items": [
        {
          "type": "object",
          "properties": {
            "a": {
              "type": "integer"
            },
            "b": {
              "type": "string"
            }
          }
        },
        {
          "type": "string"
        },
        {
          "type": "array",
          "items": {
            "type": "string"
          }
        }
      ]
    }
  }
}

... would correspond to Vec<MyEnum> where:

#[serde(untagged)]
enum MyEnum {
    MyEnumItem1 { a: i32, b: String },
    MyEnumItem2(String),
    MyEnumItem3(Vec<String>),
}

Update: I was wrong. v2 doesn't support this.

@oli-cosmian
Copy link
Contributor

We could encode single element enums as array of enums with a single element. Better than no support at all, right?

@wafflespeanut
Copy link
Collaborator

No, because that doesn't respect swagger schema and the spec emitted by us may be misinterpreted by other language-specific tools (including the official Swagger UI). So, it's actually better to have no support rather than having something ambiguous or something that misrepresents user API. That said, we "can" make this work by using vendor extensions, so that paperclip understands this in swagger (v2) spec and when we move to v3, we can use the extensions to emit the corresponding spec in v3 😅

@Ploppz
Copy link
Author

Ploppz commented Apr 22, 2020

So if I understand this correctly, paperclip only works with OpenAPI 2 at this moment, and that's why we can't do this yet?
Anything I can do to push this forward?

@wafflespeanut
Copy link
Collaborator

Yeah, we can't do this at the moment because v2 doesn't support it. We'll need v3 (which will be part of 0.5.0 release). If you have spare time to work on v3, then I'm happy to help! 😄

@Ploppz
Copy link
Author

Ploppz commented Apr 22, 2020

Yes I can make some time for this

@Ploppz
Copy link
Author

Ploppz commented Apr 23, 2020

This issue isn't specific to actix-web plugin is it?

@wafflespeanut
Copy link
Collaborator

It is. For now, we only have actix-web plugin. When we support other frameworks in the future, we can open related issues.

@Ploppz
Copy link
Author

Ploppz commented Apr 23, 2020

Just to verify my understanding of the code, isn't this more about what the general proc-macro machinery can do? Functionality has to propagate to actix-web plugin of course but still.

@wafflespeanut
Copy link
Collaborator

The proc macros that we have right now implement the traits exposed by the plugin, so they are specific to actix web. Am I addressing this correctly?

@Ploppz
Copy link
Author

Ploppz commented Apr 23, 2020

Sorry, so far I have only a shallow understanding of the code base. I thought that there was some more general functionality, then the actix layer just being sort of a thin adapter. I'll acquaint myself more with the code.

@wafflespeanut
Copy link
Collaborator

No worries! Feel free to ask as many questions as you like! I was just wondering whether I understood and addressed the question correctly. 😅

@Siphalor Siphalor linked a pull request Feb 12, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request epic A fair amount of work is involved research Some investigation required prior to making changes.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants