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

[swift5] Add support for oneOfs #8714

Merged
merged 3 commits into from
May 24, 2021
Merged

[swift5] Add support for oneOfs #8714

merged 3 commits into from
May 24, 2021

Conversation

allezxandre
Copy link
Contributor

@allezxandre allezxandre commented Feb 15, 2021

This MR relates to issue #7549, and implements support for oneOf in Swift. anyOf fields remain unsupported in this PR (despite the branch name).

Given a oneOf field named field_name, defined as:

{
    "oneOf": [
        { "$ref": "#/components/schemas/SchemaA" },
        { "$ref": "#/components/schemas/SchemaB" },
        { "$ref": "#/components/schemas/SchemaC" }
    ]
}

the generator will generate a model named FieldNameOneOf to hold the values. It is an enumeration with associated values that looks like follows:

public enum FieldNameOneOf: Codable {
    case typeSchemaA(SchemaA)
    case typeSchemaB(SchemaB)
    case typeSchemaC(SchemaC)

    public func encode(to encoder: Encoder) throws {
        var container = encoder.singleValueContainer()
        switch self {
        case .typeSchemaA(let value):
            try container.encode(value)
        case .typeSchemaB(let value):
            try container.encode(value)
        case .typeSchemaC(let value):
            try container.encode(value)
        }
    }

    public init(from decoder: Decoder) throws {
        let container = try decoder.singleValueContainer()
        if let value = try? container.decode(SchemaA.self) {
            self = .typeSchemaA(value)
        } else if let value = try? container.decode(SchemaB.self) {
            self = .typeSchemaB(value)
        } else if let value = try? container.decode(SchemaC.self) {
            self = .typeSchemaC(value)
        } else {
            throw DecodingError.typeMismatch(Self.Type.self, .init(codingPath: decoder.codingPath, debugDescription: "Unable to decode instance of FieldNameOneOf"))
        }
    }
}

Usage then goes along something like:

switch model.fieldName {
case .typeSchemaA(let value):
    // Use `value`, an object of type SchemaA
    // do something with it...
    break
case .typeSchemaB(let value):
    // do something else with value of type SchemaB...
    break
case .typeSchemaC(let value):
    // do something else with value of type SchemaC...
    break
}

oneOfs are defined by the OpenAPI spec as:

validates the value against exactly one of the subschemas

Currently, the generator does not implement validation that the value matches only one subschema, but it might be added in the future if need be.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH. → no change
  • File the PR against the correct branch: master, 5.1.x, 6.0.x → I guess this is aimed at 5.1.x, but it seems 5.1 changes are integrated into master
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request — @jgavris @ehyche @Edubits @jaz-ah @4brunu

@4brunu
Copy link
Contributor

4brunu commented Feb 16, 2021

Hey @allezxandre, thanks for this great contribution 👍
This looks very good!
Could you please just add a sample project please?
Thanks

@wing328
Copy link
Member

wing328 commented Feb 16, 2021

In the ./bin/config/swift* files, what about switching the spec from modules/openapi-generator/src/test/resources/2_0/swift/petstore-with-fake-endpoints-models-for-testing.yaml to modules/openapi-generator/src/test/resources/3_0/swift/petstore-with-fake-endpoints-models-for-testing.yaml which contains oneOf, anyOf model definitions for testing?

@wing328
Copy link
Member

wing328 commented Feb 16, 2021

Currently, the generator does not implement validation that the value matches only one subschema, but it might be added in the future if need be.

Looks like that's the implementation for anyOf. It's still a good starting point to get it going.

@allezxandre
Copy link
Contributor Author

Currently, the generator does not implement validation that the value matches only one subschema, but it might be added in the future if need be.

Looks like that's the implementation for anyOf. It's still a good starting point to get it going.

I think it's still closer to the implementation of oneOf because since it is a Swift Enum, it can only take a value of a single type — either .typeSchemaA or .typeSchemaB and so on but never more than one at the same time.

It was my understanding that a Swift implementation of anyOf would need to hold at least two types at the same time, something like this:

struct FieldAnyOf {
    var valueSchemaA: SchemaA?
    var valueSchemaB: SchemaB?
    // [...]
}

Probably not the best implementation out there, but closer to anyOf because it would be possible to hold both types.

@allezxandre
Copy link
Contributor Author

Hey @allezxandre, thanks for this great contribution 👍
This looks very good!
Could you please just add a sample project please?
Thanks

I was not really sure how to do this, so I tried something in my last two commits that I just pushed. Tell me if this is what you had in mind.

@4brunu
Copy link
Contributor

4brunu commented Feb 17, 2021

Hey @allezxandre, thanks for this great contribution 👍
This looks very good!
Could you please just add a sample project please?
Thanks

I was not really sure how to do this, so I tried something in my last two commits that I just pushed. Tell me if this is what you had in mind.

Looks good, although I don't know the different between modules/openapi-generator/src/test/resources/3_0/oneOf.yaml and modules/openapi-generator/src/test/resources/3_0/swift/petstore-with-fake-endpoints-models-for-testing.yaml that @wing328 suggested.

The last commit added a lot o noise to the PR, is that commit needed?

@4brunu
Copy link
Contributor

4brunu commented Feb 17, 2021

Currently, the generator does not implement validation that the value matches only one subschema, but it might be added in the future if need be.

Looks like that's the implementation for anyOf. It's still a good starting point to get it going.

I think it's still closer to the implementation of oneOf because since it is a Swift Enum, it can only take a value of a single type — either .typeSchemaA or .typeSchemaB and so on but never more than one at the same time.

It was my understanding that a Swift implementation of anyOf would need to hold at least two types at the same time, something like this:

struct FieldAnyOf {
    var valueSchemaA: SchemaA?
    var valueSchemaB: SchemaB?
    // [...]
}

Probably not the best implementation out there, but closer to anyOf because it would be possible to hold both types.

I also don't like that implementation a lot, but I can't think of a better one.
@allezxandre @wing328 do you know of any other generators that implement anyOf that we could pick a better ideas of the current implementations?

@allezxandre
Copy link
Contributor Author

do you know of any other generators that implement anyOf that we could pick a better ideas of the current implementations?

I haven't found any hints in the DefaultCodegen.java, it looks like most of the code just handles oneOf.

@allezxandre
Copy link
Contributor Author

The last commit added a lot o noise to the PR, is that commit needed?

I wasn't sure why there was that many files, so I just committed in a separate commit so we could drop it if it's useless. I just added the changes following instructions from the PR template:

./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh

Commit all changed files.

Tell me if you want me to drop the commit

@4brunu
Copy link
Contributor

4brunu commented Feb 17, 2021

do you know of any other generators that implement anyOf that we could pick a better ideas of the current implementations?

I haven't found any hints in the DefaultCodegen.java, it looks like most of the code just handles oneOf.

I found this implementations
#7801
#7263
#7130
#4498
#6269

@wing328
Copy link
Member

wing328 commented Feb 17, 2021

@allezxandre @wing328 do you know of any other generators that implement anyOf that we could pick a better ideas of the current implementations?

PowerShell, C#, Java, Go, etc

@kerrmarin
Copy link

kerrmarin commented Feb 17, 2021

How will this model an array of oneOf? If I understood correctly the generator will output something like:

enum OneOfAOrB {
  case a(A)
  case b(B)
}

which makes sense when it's a property, but when you have an array like [OneOfAOrB] that would break the constraint of it being one or the other? or would it generate something like...

enum CollectionOfOneOfAOrB {
  case a([A])
  case b([B])
}

Apologies if this is outside the scope of this PR, just trying to understand the change

@allezxandre
Copy link
Contributor Author

allezxandre commented Feb 17, 2021

which makes sense when it's a property, but when you have an array like [OneOfAOrB] that would break the constraint of it being one or the other? or would it generate something like...

enum CollectionOfOneOfAOrB {
  case a([A])
  case b([B])
}

I think here you are confusing an array of oneOfs with a oneOf of arrays. A oneOf of arrays type (like OneOfCollectionAOrCollectionB ) would yield what your Swift code describes: an enum of array types.
Edit: but then CollectionOfOneOfAOrB wouldn't be an appropriate name because it is a OneOfCollectionAOrCollectionB)

An array of oneOfs (like [OneOfAOrB]) would simply be an array where each element is an enum.

I hope that answers your question

@wing328 wing328 modified the milestones: 5.1.0, 5.1.1 Mar 19, 2021
@shoaibsadanaut
Copy link

@allezxandre Are you looking into it?

@allezxandre
Copy link
Contributor Author

@allezxandre Are you looking into it?

Looking into what?

This PR has a working implementation of oneOfs in Swift that I've been using in production in one App I'm building.
So in regards to a oneOf implementation in Swift, this PR is ready. (I'm not sure why the Circle CI tests fail but I don't have access to the logs unfortunately)

As for the anyOf implementation, I don't have many implementation ideas so the discussion is still open, but maybe it could make for another PR distinct of this one

@shoaibsadanaut
Copy link

@allezxandre My point was that can you try merging it before the next release? I need it for my swift project too.

@4brunu
Copy link
Contributor

4brunu commented Apr 21, 2021

@allezxandre could you please solve the merge conflicts?
And try to add to this PR only the samples related to Swift, to see if CI passes.

@allezxandre
Copy link
Contributor Author

Yes, will do soon

@4brunu
Copy link
Contributor

4brunu commented May 18, 2021

@allezxandre Hey, did you got a chance to look at the merge conflicts? Can I help in any way? 🙂

@allezxandre
Copy link
Contributor Author

Sorry, completely forgot about this.

I just dropped the commit that added samples not related to Swift, and fast-forwarded to master.
Should be good to go now, if CI passes

@4brunu
Copy link
Contributor

4brunu commented May 20, 2021

Travis CI and CircleCI failures are not related to this PR.
Drone.is is related.
Can you please run the following commands and commit the changes to fix https://cloud.drone.io/OpenAPITools/openapi-generator/16677/1/2?

./mvnw clean package 
./bin/generate-samples.sh bin/configs/swift5-*

Thanks for your help 🙂

@allezxandre
Copy link
Contributor Author

Alright, let's hope this fixes it (I don't have access to CI logs so I can't be of much help unfortunately)

@4brunu
Copy link
Contributor

4brunu commented May 21, 2021

Thanks, the remaining CI issues are not related to this PR 👍

@wing328
Copy link
Member

wing328 commented May 24, 2021

Tested with 3.0 spec (with oneOf model definitions) and the CI tests passed via https://app.bitrise.io/build/a8f360b8da5fee0d#?tab=log

@wing328 wing328 merged commit 6c40192 into OpenAPITools:master May 24, 2021
@wing328 wing328 added this to the 5.2.0 milestone May 24, 2021
premiumitsolution pushed a commit to premiumitsolution/openapi-generator that referenced this pull request May 26, 2021
* [swift5] Add support for oneOfs

* Generate a sample Swift project with oneOfs

* Update Swift Samples
@c-villain
Copy link

c-villain commented Jun 16, 2021

@allezxandre plz add support code-generation for array type in OneOf..
e.g.

case typeArray([MyArray]) - for generated code

in *.YAML - file:

oneOf:
                type: array
                items:
                    $ref: 'MyArray'

@allezxandre allezxandre deleted the swift-oneof-anyof-support branch June 16, 2021 13:08
@allezxandre
Copy link
Contributor Author

@c-villain I don't remember exactly, but I believe this was supported. Can you add a test that breaks so I can see what you're expecting?

@c-villain
Copy link

c-villain commented Jun 16, 2021

@allezxandre look plz edited comment, no, its not supported

@c-villain
Copy link

@allezxandre added bug #9785 for this issue, tried on master (currently 5.2.0), confirmed that array in "one of" not supported

@Fab1n
Copy link

Fab1n commented Oct 25, 2021

@allezxandre this feature does not work with the hashableModels cli config option which is true by default. Hashable conformance is added to all models except the enums I think. Why this wasn't an issue in the tests, I don't know... Maybe there are no tests for this case? (Although hashableModels is a default parameter, which needs to be testet in any case, right?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants