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

Union type of primitives does not generate value serialialization code. #2462

Closed
darrelmiller opened this issue Mar 26, 2023 · 7 comments · Fixed by #2826
Closed

Union type of primitives does not generate value serialialization code. #2462

darrelmiller opened this issue Mar 26, 2023 · 7 comments · Fixed by #2826
Assignees
Labels
generator Issues or improvements relater to generation capabilities. type:bug A broken experience WIP
Milestone

Comments

@darrelmiller
Copy link
Member

Using this example:

openapi: 3.0.1
info:
    title: Example of UnionTypes
    version: 1.0.0
paths:
  /primitives:
    get:
      responses:
        '200':
          description: OK
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/primitives'
components:
  schemas:
    primitives:
      oneOf:
        - type: string
        - type: number

Generates this model serialization code:

        public void Serialize(ISerializationWriter writer) {
            _ = writer ?? throw new ArgumentNullException(nameof(writer));
            if(Double != null) {
                writer.WriteDoubleValue(null, Double);
            }
            else if(String != null) {
                writer.WriteStringValue(null, String);
            }
            writer.WriteAdditionalData(AdditionalData);
        }

The null values passed to WriteDoubleValue and WriteStringValue should be string literals "Double" and "String".

@baywet
Copy link
Member

baywet commented Mar 26, 2023

The first parameter is the key, left to null here because it's a scalar value. The second parameter is the value. I'm not sure I follow what's wrong here besides the fact that we might want to avoid including additional data.
Can you provide additional context please?

@darrelmiller
Copy link
Member Author

When the serializer tries to serialize this object it fails because null is not a valid property name.

Unhandled exception: System.InvalidOperationException: Cannot write a JSON value within an object without a property name. Current token type is 'StartObject'.
   at System.Text.Json.ThrowHelper.ThrowInvalidOperationException(ExceptionResource resource, Int32 currentDepth, Int32 maxDepth, Byte token, JsonTokenType tokenType)
   at System.Text.Json.Utf8JsonWriter.ValidateWritingValue()
   at System.Text.Json.Utf8JsonWriter.WriteStringByOptions(ReadOnlySpan`1 value)
   at System.Text.Json.Utf8JsonWriter.WriteStringEscape(ReadOnlySpan`1 value)
   at System.Text.Json.Utf8JsonWriter.WriteStringValue(ReadOnlySpan`1 value)
   at System.Text.Json.Utf8JsonWriter.WriteStringValue(String value)
   at Microsoft.Kiota.Serialization.Json.JsonSerializationWriter.WriteStringValue(String key, String

@baywet
Copy link
Member

baywet commented Mar 27, 2023

This is most likely because Request information is writing an object and then the serialization writer starts an object no matter what

We should probably introduce an interface for composed types wrappers so the start and end object can be added conditionally.

@baywet baywet assigned baywet and unassigned darrelmiller Mar 27, 2023
@baywet baywet added type:bug A broken experience generator Issues or improvements relater to generation capabilities. and removed Needs: Attention 👋 labels Mar 27, 2023
@baywet baywet added this to Kiota Mar 27, 2023
@github-project-automation github-project-automation bot moved this to Todo in Kiota Mar 27, 2023
@baywet baywet modified the milestones: Backlog, Kiota v1.2 Mar 27, 2023
@baywet
Copy link
Member

baywet commented Jun 22, 2023

Introducing a new interface to "mark" composed types is that it won't work for any weakly type language, or language that works with type assertions.

Introducing new methods in the serialization writer (WriteUnionType) interface is probably a bad idea as well as:

  • it'd be a breaking change for Go and C#
  • a union type can be a mix of primitive and complex types, and we're already generating code that tests the different properties. We'd get duplication and removing the duplication could lead to having to rely on reflection.

Annotations are probably also a bad bet since it's not supported in most languages and would lead to using reflection.

The only solution I can think of within the constraints would be to catch the exception / check when we're adding a "property" with no name and the previous character is opening an object (curly). If that's the case, erase the previous character and then proceed. But this feels like a really dirty hack...

@baywet
Copy link
Member

baywet commented Jun 22, 2023

Just for clarity over the last comments the sequence is

  1. WriteObjectValue (kiota json serialization writer)
  2. WriteObjectStart (Json.net)
  3. Serialize (generated code)
  4. WriteScalarValue (kiota json serialization writer)
  5. WriteScalarValue (json.net)
  6. exception

@baywet
Copy link
Member

baywet commented Jun 22, 2023

To solve this properly maybe we could introduce a new interface

public interface IComposedTypeWrapper
{
    IsScalarValue { get; }
}

This would lead to generation of the same evaluation process as the serialize method, but that would just return a boolean value instead of writing anything yet.

This way step 1 of the previous answer could type assert/test for the presence of the getter/method, if it's here call it, and if the result is true, skip the object start before calling serialize.

@baywet baywet moved this from Todo to In Progress in Kiota Jun 22, 2023
@baywet
Copy link
Member

baywet commented Jun 26, 2023

After further reflection, that method/property doesn't need any logic/can only return true/doesn't need to exist. As for composed types serialize method call into WriteObjectValue itself (creating two nested objects if we don't skip the first one).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
generator Issues or improvements relater to generation capabilities. type:bug A broken experience WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants