Skip to content

Comments

Fixes for JSON merge patch in polymorphic types#2775

Merged
alzimmermsft merged 8 commits intoAzure:mainfrom
alzimmermsft:StreamStyleJsonPatchChanges
May 24, 2024
Merged

Fixes for JSON merge patch in polymorphic types#2775
alzimmermsft merged 8 commits intoAzure:mainfrom
alzimmermsft:StreamStyleJsonPatchChanges

Conversation

@alzimmermsft
Copy link
Member

@alzimmermsft alzimmermsft commented May 17, 2024

Fixes #2769
Fixes #2770
Fixes #2771

Some fixes when handling stream-style, polymorphic, JSON merge patch models.

  • Reduced the number of access helper interfaces created to only the polymorphic parent type. The polymorphic parent will manage whether JSON merge patch serialization will be used and will share this information to child types through an access helper method.
  • Change how BinaryData is created to just use BinaryData.fromObject. To make sure serialization happens before JSON merge patch serialization is disabled BinaryData.getLength() is called instead of the previous BinaryData.fromBytes(BinaryData.fromObject(object).toBytes()) pattern, this will retain information in BinaryData that it is based on an object.
  • Changed how deserialization works to no longer call the setter methods when the property is defined by a parent model. This was accidentally setting those properties to always be included in JSON merge patch serialization, which could lead to incorrect behavior. Now the parent models expose access helper methods for setting properties which child models will set directly.
  • Cleaned up deserialization code when a JSON merge patch model has no parameters in the constructor. Before this was just checking if the model had required properties and required properties were being included in the constructor, but JSON merge patch models don't generate required properties in the constructor if they're optional in the JSON merge patch call.

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft left a comment

Choose a reason for hiding this comment

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

@haolingdong-msft
You are the owner of patch feature. We will need you to sign off.

continue;
}

if (model.isPolymorphic() && CoreUtils.isNullOrEmpty(model.getDerivedModels())) {
Copy link
Member

@weidongxu-microsoft weidongxu-microsoft May 22, 2024

Choose a reason for hiding this comment

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

Just a notice: criteria like "CoreUtils.isNullOrEmpty(model.getDerivedModels()) => polymorphic parent" that works for single-level polymorphic won't work for multi-level discriminator -- not a blocker as existing code likely already having similar problems

Also, in Swagger there could be (still single discriminator)

classDiagram
  base <|-- level1Terminal 
  base <|-- level1NonTerminal 
  level1NonTerminal <|-- level2Terminal
  class base {
    +kind/discriminator 
  }
  class level1NonTerminal  {
    +property1
  }
  class level1Terminal {
    kind=terminal1
  }
  class level2Terminal {
    kind=terminal2
  }
Loading

Does level1NonTerminal.property1 need to have accessor?

Copy link
Member Author

Choose a reason for hiding this comment

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

This should work in this scenario and the intermediate polymorphic parent will need accessors for all its new properties so that level2Terminal can set those values during deserialization.

I'll update patch.tsp to add this scenario so it can be validated.

Choose a reason for hiding this comment

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

Yeah, I think current logic is good.

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft May 23, 2024

Choose a reason for hiding this comment

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

What's the difference of this line and model.isPolymorphic() && !model.isPolymorphicParent()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, doesn't seem like too much now that I look at it again. I got caught up on model.isPolymorphicParent() checking both model.isPolymorphic() && !CoreUtils.isNullOrEmpty(model.getDerivedModels()), so it ends up being the same in this case. I likely overlooked this as my first design used !model.isPolymorphicParent() by itself, which would have been wrong here as non-polymorphic models would have passed the check.

@haolingdong-msft
Copy link
Member

The getLength() change to fire serialization and removing extra code in the accessor look good to me.

@alzimmermsft alzimmermsft merged commit f15296b into Azure:main May 24, 2024
@alzimmermsft alzimmermsft deleted the StreamStyleJsonPatchChanges branch May 24, 2024 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants