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

CompositeType parameters incorrectly being marked as constants #819

Closed
annatisch opened this issue Mar 8, 2016 · 5 comments
Closed

CompositeType parameters incorrectly being marked as constants #819

annatisch opened this issue Mar 8, 2016 · 5 comments

Comments

@annatisch
Copy link
Member

Hi - I'm seeing some strange behavior where non-constant complex type parameters are being incorrectly described as constant.

As example can be seen when generating this swagger spec:
https://github.com/Azure/azure-rest-api-specs/blob/master/arm-network/2015-06-15/swagger/network.json
In particular, the model NetworkInterfaceIPConfiguration has a complex parameter Subnet - subnet is being classified as "constant" when I can't see a reason that it should be (i.e. it's not a string enum of one value, nor are all of it's parameters constant).

I stepped through the code, and I see that things seem to be going wrong in AutoRest.Modeler.Swagger.ObjectBuilder - line 169. It seems that in this particular scenario, compositeType.ComposedProperties is empty, so the if statement passes and the model is marked as constant.
I can fix this particular issue by simply replacing the code with:

var compositeType = parameter.Type as CompositeType;
if (compositeType != null and compositeType.ComposedProperties.Any())
{
    if (compositeType.ComposedProperties.All(p => c.IsConstant))
    {
        parameter.DefaultValue = "{}";
        parameter.IsConstant = true;
    }
}

However I don't know what other implications this change may have, and the bigger problem seems to be why ComposedProperties was empty in the first place (which I didn't investigate)....
Anyways - I hope this helps! We have some customers who are affected by this issue due to how we handle constants in the Python generator. If this is determined to not be a bug, let me know and I can change the Python implementation to work around this scenario.

Thanks!

@lmazuel
Copy link
Member

lmazuel commented Mar 8, 2016

For the record, this created this issue in the Python SDK:
Azure/azure-sdk-for-python#536

@devigned devigned added the bug label Mar 14, 2016
@annatisch
Copy link
Member Author

We need a fix for this issue pretty soon @devigned and @yugangw-msft .
Would it help if I submitted the workaround above as a PR? I don't know if it's the correct approach or what impact it may have on the other languages, so your thoughts would be appreciated :)

@amarzavery
Copy link
Contributor

@annatisch - That is a correct fix. I have verified this fix with Azure.NodeJS on the network spec as well. After applying your fix the generated code looked better. CompositeTypes were not being marked as constant where they shouldn't be.
Just one comment:
if (compositeType != null && compositeType.ComposedProperties.Any())

Please send a PR, and possibly a test in the modeler over here to make sure that this does not regress.
You can create a new swagger doc with few model classes from the network swagger spec that reproduce this problem and then assert that subnet is not a constant property in NetworkInterfaceIPConfiguration class.

@annatisch
Copy link
Member Author

Aww but the Python 'and' is so much more elegant than the C# '&&'... ;P
Thanks - I'll try and get a PR out tomorrow.

@amarzavery
Copy link
Contributor

I agree. I used to write in Perl and loved that elegance. However this is still better than PowersHell -gt, -lt, -eq :)

amarzavery added a commit that referenced this issue Mar 16, 2016
jianghaolu added a commit to jianghaolu/AutoRest that referenced this issue Jun 28, 2016
cf2fdb5 Adding publicIp sample, only mark sub-DAGs as non-prepare - the root DAG stay as preparer
1d5e9c1 improving comment as per review
25beef0 Addressing review comments [javadoc, simplfying the map iteration]
cfbafd0 comment corrections
ac633da Making DAG to work correctly to handle more cases
45c3d88 couple of javadoc fixes
57288f4 Merge pull request Azure#819 from jianghaolu/async
999fb18 Add retry for 404 GET
9c048fc Add a bunch of configs to RestClient
46a53cd Merge branch 'master' of github.com:Azure/azure-sdk-for-java into async
76e3ae9 Merge pull request Azure#832 from jianghaolu/restclient
7be25fe Merge commit '486acdc633c498d4335711e30817cc70faacc487' into restclient
04af976 Regenerate azure & azure.fluent
fecda5c Move RestClient into azure runtime
f84d8eb making dag::prepare to work for update
05105b4 Remove rest client from generic codegen
bb4a917 Fixing the bug of trying to create created resources during update, adding better way to name gen resource names
1773d49 Add stage for base url
4a7b25e Remove required mapper adapter
738d121 use AZURE_GERMANY insteadof AZURE_GERMAN
6c8f63a ensuring trailing slash for urls
c8db8d3 correcting environment name
0261a6e Adding few more Azure environments
5aac1fa Add applyAsync()
bc57070 Basic prototyping of async in fluent

git-subtree-dir: ClientRuntimes/Java
git-subtree-split: cf2fdb582fb504ea5f772ed03cffb07662c8c626
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants