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

Skip model generation if it's a top-level map or array #1296

Merged
merged 16 commits into from
Nov 26, 2018
Merged

Conversation

wing328
Copy link
Member

@wing328 wing328 commented Oct 23, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Skip model generation if it's a top-level "additionalProperties' (an alias to "additionalProperties")

Closes #391

cc @OpenAPITools/generator-core-team

@wing328 wing328 changed the title Skip model generation if it's a top-level "additionalProperties' Skip model generation if it's a top-level "additionalProperties" (map) Oct 23, 2018
@wing328 wing328 changed the title Skip model generation if it's a top-level "additionalProperties" (map) Skip model generation if it's a top-level map or array Oct 24, 2018
@etherealjoy
Copy link
Contributor

etherealjoy commented Oct 24, 2018

@wing328
It worked very well apparently.
This issue #922 is solved in this case
But it generated the inline ref to array model as AmfUpdateEventSubscriptionItem_inner and the following log is generated

[main] INFO  o.o.codegen.DefaultGenerator - Model AmfUpdateEventSubscriptionItem not 
generated since it's an alias to array (without property)

I think here is a matter of choice how we want to do for alias, if we use refname or inner

However it does not apply this behavior when the array/map alias is part of a model like here #1251/#1273

We can do this as a next steps.
Thanks a lot of the update.

@wing328
Copy link
Member Author

wing328 commented Oct 24, 2018

@etherealjoy thanks for the review.

cc @OpenAPITools/generator-core-team

@etherealjoy
Copy link
Contributor

Fixes #397/#922

@etherealjoy
Copy link
Contributor

etherealjoy commented Oct 24, 2018

I have a regression here with #391 :(
go client does not build. go server does build.

EDIT:
I think mainly client generators are affected in both map and array aliases.

@wing328
Copy link
Member Author

wing328 commented Oct 24, 2018

@etherealjoy I'll take a look tomorrow to see if I can hunt down the bugs

@etherealjoy
Copy link
Contributor

etherealjoy commented Nov 23, 2018

@wing328
Also solves #397 #922
Still to be solved for non top aliases like #1251/#1273

@wing328
Copy link
Member Author

wing328 commented Nov 24, 2018

I pushed another fix and got the following (different) code snippet for fromJson method when using the spec in #1273

void UeContext::fromJson(const nlohmann::json& val)
{
    {
        m_Groups.clear();
        if(val.find("groups") != val.end())
        {
            for( auto& item : val["groups"] )
            {
                m_Groups.push_back(item);

            }
        }
    }

}

Please give it another try when you've time.

@etherealjoy
Copy link
Contributor

etherealjoy commented Nov 24, 2018

@wing328
This is nice, I compiled #1273 successfully.
Solves #1273, #397, #922, #391

#1251 need OAS3 support as well so not in this scope.

@eriktim
Copy link
Contributor

eriktim commented Nov 24, 2018

Fixes #1465 as well!

@wing328
Copy link
Member Author

wing328 commented Nov 25, 2018

If no further feedback on this PR, I'll merge this on coming Monday.

@CyrilleBenard
Copy link

I pushed another fix and got the following (different) code snippet for fromJson method when using the spec in #1273

void UeContext::fromJson(const nlohmann::json& val)
{
    {
        m_Groups.clear();
        if(val.find("groups") != val.end())
        {
            for( auto& item : val["groups"] )
            {
                m_Groups.push_back(item);

            }
        }
    }

}

Please give it another try when you've time.

I still got this kind of wrong generated code with current master (3.3.4) :/

@etherealjoy
Copy link
Contributor

etherealjoy commented Nov 29, 2018

@CyrilleBenard
This is correct

std::vector<std::string> m_Groups;

m_Groups.clear();
if(val.find("groups") != val.end())
{
      for( auto& item : val["groups"] )
      {
            m_Groups.push_back(item);         
      }
}
    UeContext:
      type: object
      properties:
        groups:
          type: array
          items:
            $ref: '#/components/schemas/GroupId'

    GroupId:
      type: string

@CyrilleBenard
Copy link

Oh sorry I omitted m_Groups was a string. I got the same kind of issue when m_Groups is a type defined by a model class. I'm going to reproduce the issue in a small context usage and will open another bug issue.

@etherealjoy
Copy link
Contributor

@CyrilleBenard
OK, please do.
Could just then be specific to the generator.

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
…1296)

* update samples

* remove string boolean map spec

* add logic to skip array alias being generated as model

* fix alias to array

* remove unused ruby files

* remove unused ruby (oas3) files

* unalias response schema

* fix NPE when no model defined

* fix ruby openapi3 script

* update samples

* add global openapi, schemas for unaliasing

* minor code cleanup/refactoring using globalSchemas

* Revert "minor code cleanup/refactoring using globalSchemas"

This reverts commit 20a2bbc.
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.

4 participants