[Python] Initial fixes using simple `oneOf`s with discriminator #4435

andreaTP · 2024-04-02T18:41:18Z

andreaTP · 2024-04-09T18:12:15Z

src/Kiota.Builder/CodeDOM/CodeClass.cs

 {
+ // Prevent duplicates


Those changes are fixing #4178

Please note that two properties with colliding serialization names are going to collide anyway causing just an even more subtle runtime error.
I believe this is an improvement that doesn't impact code that was already correct, but it would be great to test the changes on the Graph API to confirm.

To test in other projects I used a pre-release pipeline:
https://github.com/andreaTP/kiota-prerelease/releases/tag/v0.0.0-pre%2BandreaTP.initial-fixes-simple-oneOf-py.b46369a

@andreaTP The PR looks good from my end. I just need to run some tests on the graph api metadata and I'll confirm if everything is good here.

🙏 thanks!

Looking at microsoftgraph/msgraph-sdk-dotnet@dev...kiota/v1.0/pipelinebuild/142930

It looks like there's something we need to rethink here. It looks like kiota expects the property to be added to the codeDom either way if it exists up the hierarchy.

The relevant writer/refiner of the language will then use the ExistsInBaseType property to determine whether the property should be redifined in the child type based on the language semantics. It also allows the property to hold multiple values based on the context(if we are in the base or child class) in scenarios for default values.

The groupby here is fine, it'd prevent somebody from adding twice the same property from the same schema. Unlikely because of the conventions of JSON schema, but fine.
What's really problematic here is this if block, it effectively breaks behaviour with overridden defaults in derived types. Can you provide more context on why do you think it's necessary in an oneOf scenario @andreaTP please?

Without the check, a discriminator property is added multiple times (as reported in other issues) and it breaks serialization/deserialization.

and that discriminator property being added multiple times only happens whenever we have a union type?

I don't know, it happens for sure when we have a union type, if this is the only case I cannot tell.
IIRC the discriminator gets added +2 times anytime the schema declaring it appears in the OpenAPI definition.

You can verify the behavior by using the reproducer: https://github.com/microsoft/kiota/pull/4435/files#diff-2862479147a7917233ba0a4bc0e346b2e297c170b54b431a1c3f511765e2ca53

andreaTP · 2024-04-09T18:24:02Z

@baywet @andrueastman to your attention as I hope we can get this one(or the resolution of the linked issues in another form) at your earliest 🙏

andreaTP · 2024-04-11T12:54:30Z

The "build" failure is correct as I haven't finished fixing the unit tests according to the new implementation 😅 doing it now.

andreaTP · 2024-04-11T17:40:10Z

Moving back to draft as I need a little guidance on the failing tests, I think that I'm missing an invariant, but struggling to get it right for all the use cases.

andreaTP · 2024-04-11T17:40:46Z

if anyone of the maintainers is available to point me in the right direction or helping out it would be much appreciated 🙏

andrueastman · 2024-04-12T08:13:09Z

if anyone of the maintainers is available to point me in the right direction or helping out it would be much appreciated 🙏

#4435 (comment)

andreaTP · 2024-04-18T11:52:40Z

Apologize I forgot to follow up, @baywet I think that I do not fully understand all the use cases, it will be great if you can look into this 🙏

baywet

Thanks for starting this.
@andreaTP Do you have any specific question regarding the guidance you're looking for here? Through reviewing the issue and the PR I couldn't see any outstanding questions?

baywet · 2024-04-22T17:09:19Z

CHANGELOG.md

@@ -35,6 +35,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Changed Csharp code generation to put braces on new lines (where it makes sense). [#4347](https://github.com/microsoft/kiota/issues/4347)
 - Fixed a bug where some no-content status codes would be considered structured (301, 302, 303, 307) when described. [#4190](https://github.com/microsoft/kiota/issues/4190)
 - TypeScript is now a preview language!
+- Fixed a bug where the code generated for Python in presence of discriminator values was not compiling/working. [#4433](https://github.com/microsoft/kiota/issues/4433)


this needs to be moved to the unreleased section

ok, I'll do it

baywet · 2024-04-22T17:25:58Z

src/Kiota.Builder/Writers/Python/CodeMethodWriter.cs

+ var className = parentClass.Name;
+ if (parentClass.Parent is CodeClass pc &&
+ !string.IsNullOrEmpty(pc.Name) &&
+ pc.InnerClasses.Any(x => x == parentClass))
+ {
+ className = $"{pc.Name}.{parentClass.Name}";
+ }
+ writer.WriteLine($"{ResultVarName} = {className}()");


I'm struggling to understand what's happening here. By definition the wrapper class cannot/shouldn't be one of the member types?

🤷‍♂️ I just changed the code that was not compiling, not sure if there is a more principled fix for the issue.

src/Kiota.Builder/CodeDOM/CodeClass.cs

Co-authored-by: Vincent Biret <vibiret@microsoft.com>

andreaTP · 2024-04-23T09:24:18Z

Do you have any specific question regarding the guidance you're looking for here? Through reviewing the issue and the PR I couldn't see any outstanding questions?

@baywet thanks for getting back on this!
Some Unit tests are breaking too, but I'm not sure I understand what's the desired behavior in all the cases.
As demonstrated here: #4435 (comment) this PR is breaking "something" but I'm unable to understand what and how to fix it.

If you can have a look at the failing unit tests you can probably give me some answers ...

baywet · 2024-04-24T16:17:17Z

@andreaTP I fixed all the failing unit tests by simply removing the if block I was referring to here but didn't push my changes as I wanted to get more context on the matter first.
I think it's be important to get more context on why you think this if block is needed, and maybe narrow down the scenario so the fix can be adapted to be more specific in order to avoid undesired side effects.

andreaTP · 2024-04-24T16:28:08Z

@baywet thanks a lot for checking this one!
Have you tested with the newly introduced Python it test?
I bet you get a failure there and we can go on from there, I assume that the generated code and a StackTrace are going to be worth a thousand words.

Alternatively, I can get back to this early next week.

andreaTP · 2024-05-07T12:53:43Z

For the records, this is still an issue as far as I can tell, but it dropped on my priority list and it would be great if someone would take over.

andreaTP requested a review from a team as a code owner April 2, 2024 18:41

This comment was marked as outdated.

Sign in to view

andreaTP marked this pull request as draft April 3, 2024 15:01

andreaTP added 9 commits April 9, 2024 18:29

Get simple oneOf to work in Python

4a19c6a

test in ci

944ef1a

more fixes

2396b63

exclude typescript for now

8a7a778

changelog

56679bf

cleanup

95342cf

More debugging

adc2076

more fixes

b2d4971

testme

5112a5a

andreaTP force-pushed the initial-fixes-simple-oneOf-py branch from 3a15f0a to 5112a5a Compare April 9, 2024 17:29

cleanup

b46369a

andreaTP marked this pull request as ready for review April 9, 2024 18:07

andreaTP commented Apr 9, 2024

View reviewed changes

Merge branch 'main' into initial-fixes-simple-oneOf-py

c3f3d38

andrueastman self-assigned this Apr 11, 2024

more tests

4eb8ff7

andreaTP marked this pull request as draft April 11, 2024 17:40

baywet requested changes Apr 22, 2024

View reviewed changes

Update src/Kiota.Builder/CodeDOM/CodeClass.cs

474f19a

Co-authored-by: Vincent Biret <vibiret@microsoft.com>

Merge branch 'main' into initial-fixes-simple-oneOf-py

c4d8e02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Python] Initial fixes using simple `oneOf`s with discriminator #4435

[Python] Initial fixes using simple `oneOf`s with discriminator #4435

andreaTP commented Apr 2, 2024

This comment was marked as outdated.

andreaTP Apr 9, 2024

andreaTP Apr 9, 2024

andrueastman Apr 11, 2024

andreaTP Apr 11, 2024

andrueastman Apr 12, 2024

baywet Apr 23, 2024

andreaTP Apr 23, 2024

baywet Apr 23, 2024

andreaTP Apr 23, 2024

andreaTP Apr 23, 2024

andreaTP commented Apr 9, 2024

andreaTP commented Apr 11, 2024

andreaTP commented Apr 11, 2024

andreaTP commented Apr 11, 2024

andrueastman commented Apr 12, 2024

andreaTP commented Apr 18, 2024

baywet left a comment

baywet Apr 22, 2024

andreaTP Apr 23, 2024

baywet Apr 22, 2024

andreaTP Apr 23, 2024

andreaTP commented Apr 23, 2024

baywet commented Apr 24, 2024

andreaTP commented Apr 24, 2024

andreaTP commented May 7, 2024

[Python] Initial fixes using simple oneOfs with discriminator #4435

Are you sure you want to change the base?

[Python] Initial fixes using simple oneOfs with discriminator #4435

Conversation

andreaTP commented Apr 2, 2024

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaTP commented Apr 9, 2024

andreaTP commented Apr 11, 2024

andreaTP commented Apr 11, 2024

andreaTP commented Apr 11, 2024

andrueastman commented Apr 12, 2024

andreaTP commented Apr 18, 2024

baywet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaTP commented Apr 23, 2024

baywet commented Apr 24, 2024

andreaTP commented Apr 24, 2024

andreaTP commented May 7, 2024

[Python] Initial fixes using simple `oneOf`s with discriminator #4435

[Python] Initial fixes using simple `oneOf`s with discriminator #4435