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

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

Closed
wants to merge 14 commits into from
1 change: 1 addition & 0 deletions .github/workflows/integration-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ jobs:
- "./tests/Kiota.Builder.IntegrationTests/NoUnderscoresInModel.yaml"
- "./tests/Kiota.Builder.IntegrationTests/ToDoApi.yaml"
- "./tests/Kiota.Builder.IntegrationTests/GeneratesUritemplateHints.yaml"
- "./tests/Kiota.Builder.IntegrationTests/DiscriminatorSample.yaml"
- "oas::petstore"
- "apisguru::twitter.com:current"
- "apisguru::notion.com"
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be moved to the unreleased section

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I'll do it


## [1.12.0] - 2024-03-06

Expand Down
21 changes: 16 additions & 5 deletions it/config.json
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,19 @@
}
]
},
"./tests/Kiota.Builder.IntegrationTests/DiscriminatorSample.yaml": {
"MockServerITFolder": "discriminator",
"Suppressions": [
{
"Language": "ruby",
"Rationale": "https://github.com/microsoft/kiota/issues/2484"
},
{
"Language": "typescript",
"Rationale": "https://github.com/microsoft/kiota/issues/4434"
}
]
},
"./tests/Kiota.Builder.IntegrationTests/InheritingErrors.yaml": {
"MockServerITFolder": "basic",
"Suppressions": [
Expand Down Expand Up @@ -341,9 +354,7 @@
]
},
"apisguru::apis.guru": {
"Suppressions": [
],
"IdempotencySuppressions": [
]
"Suppressions": [],
"IdempotencySuppressions": []
}
}
}
56 changes: 56 additions & 0 deletions it/python/discriminator/test_serdeser.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import pytest

from kiota_serialization_json.json_parse_node_factory import JsonParseNodeFactory
from kiota_serialization_json.json_serialization_writer_factory import JsonSerializationWriterFactory

from client.models.component import Component
from client.models.component1 import Component1
from client.models.component2 import Component2

@pytest.mark.asyncio
async def test_component1_deser():
factory = JsonParseNodeFactory()
root = factory.get_root_parse_node('application/json', '{"objectType": "obj1", "one": "foo"}'.encode('utf-8'))
result = root.get_object_value(Component)
assert hasattr(result, "component1")
assert not hasattr(result, "component2")
assert isinstance(result.component1, Component1)
assert result.component1.object_type == "obj1"
assert result.component1.one == "foo"

@pytest.mark.asyncio
async def test_component2_deser():
factory = JsonParseNodeFactory()
root = factory.get_root_parse_node('application/json', '{"objectType": "obj2", "two": "bar"}'.encode('utf-8'))
result = root.get_object_value(Component)
assert hasattr(result, "component2")
assert not hasattr(result, "component1")
assert isinstance(result.component2, Component2)
assert result.component2.object_type == "obj2"
assert result.component2.two == "bar"

@pytest.mark.asyncio
async def test_component1_ser():
obj = Component()
obj1 = Component1()
obj1.object_type = "obj1"
obj1.one = "foo"
obj.component1 = obj1
factory = JsonSerializationWriterFactory()
writer = factory.get_serialization_writer('application/json')
obj.serialize(writer)
content = writer.get_serialized_content().decode('utf-8')
assert content == '{"objectType": "obj1", "one": "foo"}'

@pytest.mark.asyncio
async def test_component2_ser():
obj = Component()
obj2 = Component2()
obj2.object_type = "obj2"
obj2.two = "bar"
obj.component2 = obj2
factory = JsonSerializationWriterFactory()
writer = factory.get_serialization_writer('application/json')
obj.serialize(writer)
content = writer.get_serialized_content().decode('utf-8')
assert content == '{"objectType": "obj2", "two": "bar"}'
2 changes: 0 additions & 2 deletions it/python/requirements-dev.txt
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,5 @@ six==1.16.0 ; python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'

sniffio==1.3.1 ; python_version >= '3.7'

uritemplate==4.1.1 ; python_version >= '3.6'

yarl==1.9.4 ; python_version >= '3.7'

10 changes: 8 additions & 2 deletions src/Kiota.Builder/CodeDOM/CodeClass.cs
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,14 @@ public override IEnumerable<CodeProperty> AddProperty(params CodeProperty[] prop
if (properties.Length == 0)
throw new ArgumentOutOfRangeException(nameof(properties));

return properties.Select(property =>
return properties.GroupBy(static x => x.Name).Select(static x => x.First()).Select(property =>
andreaTP marked this conversation as resolved.
Show resolved Hide resolved
{
// Prevent duplicates
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏 thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (FindPropertyByNameInTypeHierarchy(property.Name) != null)
{
return null; // this property is going to collide, skipping
}

if (property.IsOfKind(CodePropertyKind.Custom, CodePropertyKind.QueryParameter))
{
if (GetOriginalPropertyDefinedFromBaseType(property.WireName) is CodeProperty original)
Expand All @@ -98,7 +104,7 @@ public override IEnumerable<CodeProperty> AddProperty(params CodeProperty[] prop
}
var result = base.AddProperty(property).First();
return PropertiesByWireName.GetOrAdd(result.WireName, result);
}).ToArray();
}).OfType<CodeProperty>().ToArray();
}
public override void RenameChildElement(string oldName, string newName)
{
Expand Down
15 changes: 11 additions & 4 deletions src/Kiota.Builder/Writers/Python/CodeMethodWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,14 @@ private void WriteFactoryMethodBodyForInheritedModel(CodeClass parentClass, Lang
private const string ResultVarName = "result";
private void WriteFactoryMethodBodyForUnionModel(CodeMethod codeElement, CodeClass parentClass, CodeParameter parseNodeParameter, LanguageWriter writer)
{
writer.WriteLine($"{ResultVarName} = {parentClass.Name}()");
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}()");
Comment on lines +140 to +147
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var includeElse = false;
foreach (var property in parentClass.GetPropertiesOfKind(CodePropertyKind.Custom)
.OrderBy(static x => x, CodePropertyTypeForwardComparer)
Expand Down Expand Up @@ -508,7 +515,7 @@ private void WriteDeserializerBodyForUnionModel(CodeMethod method, CodeClass par
.ThenBy(static x => x.Name)
.Select(static x => x.Name))
{
writer.StartBlock($"if self.{otherPropName}:");
writer.StartBlock($"if hasattr(self, \"{otherPropName}\"):");
writer.WriteLine($"return self.{otherPropName}.{method.Name}()");
writer.DecreaseIndent();
}
Expand Down Expand Up @@ -659,7 +666,7 @@ private void WriteSerializerBodyForUnionModel(CodeClass parentClass, LanguageWri
.OrderBy(static x => x, CodePropertyTypeForwardComparer)
.ThenBy(static x => x.Name))
{
writer.StartBlock($"{(includeElse ? "el" : string.Empty)}if self.{otherProp.Name}:");
writer.StartBlock($"{(includeElse ? "el" : string.Empty)}if hasattr(self, \"{otherProp.Name}\"):");
writer.WriteLine($"writer.{GetSerializationMethodName(otherProp.Type)}(None, self.{otherProp.Name})");
writer.DecreaseIndent();
if (!includeElse)
Expand All @@ -676,7 +683,7 @@ private void WriteSerializerBodyForIntersectionModel(CodeClass parentClass, Lang
.OrderBy(static x => x, CodePropertyTypeBackwardComparer)
.ThenBy(static x => x.Name))
{
writer.StartBlock($"{(includeElse ? "el" : string.Empty)}if self.{otherProp.Name}:");
writer.StartBlock($"{(includeElse ? "el" : string.Empty)}if hasattr(self, \"{otherProp.Name}\"):");
writer.WriteLine($"writer.{GetSerializationMethodName(otherProp.Type)}(None, self.{otherProp.Name})");
writer.DecreaseIndent();
if (!includeElse)
Expand Down
51 changes: 51 additions & 0 deletions tests/Kiota.Builder.IntegrationTests/DiscriminatorSample.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
openapi: 3.0.0
info:
title: "Discriminator API"
version: "1.0.0"
servers:
- url: https://mytodos.doesnotexist/
paths:
/discriminateme:
post:
description: Return something
responses:
"200":
description: OK
content:
application/json:
schema:
$ref: "#/components/schemas/Component"
requestBody:
content:
application/json:
schema:
$ref: "#/components/schemas/Component"
components:
schemas:
Component:
oneOf:
- $ref: "#/components/schemas/Component1"
- $ref: "#/components/schemas/Component2"
discriminator:
propertyName: objectType
mapping:
obj1: "#/components/schemas/Component1"
obj2: "#/components/schemas/Component2"
Component1:
type: object
required:
- objectType
properties:
objectType:
type: string
one:
type: string
Component2:
type: object
required:
- objectType
properties:
objectType:
type: string
two:
type: string
Original file line number Diff line number Diff line change
Expand Up @@ -811,7 +811,7 @@ public void WritesUnionDeSerializerBody()
Assert.DoesNotContain("super_fields = super()", result);
Assert.DoesNotContain("return fields", result);
Assert.DoesNotContain("elif", result);
Assert.Contains("if self.complex_type1_value:", result);
Assert.Contains("if hasattr(self, \"complex_type1_value\"):", result);
Assert.Contains("return self.complex_type1_value.get_field_deserializers()", result);
Assert.Contains("return {}", result);
}
Expand Down