Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
changeKind: fix
packages:
- "@typespec/http-client-java"
---

Prevent duplicate Java discriminator members while preserving inherited discriminators in stream-style XML serialization.
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public final class GoblinShark extends Shark {
* Discriminator property for Fish.
*/
@Metadata(properties = { MetadataProperties.GENERATED })
private String kind = "shark";
private final String kind = "shark";

/*
* The sharktype property.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public final class SawShark extends Shark {
* Discriminator property for Fish.
*/
@Metadata(properties = { MetadataProperties.GENERATED })
private String kind = "shark";
private final String kind = "shark";

/*
* The sharktype property.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public class Shark extends Fish {
* Discriminator property for Fish.
*/
@Metadata(properties = { MetadataProperties.GENERATED })
private String kind = "shark";
private final String kind = "shark";

/*
* The sharktype property.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.function.BiConsumer;
import java.util.function.Consumer;
import java.util.stream.Collectors;
import java.util.stream.Stream;

/**
* Manages metadata about properties in a {@link ClientModel} and how they correlate with model class generation.
Expand Down Expand Up @@ -124,10 +125,11 @@ public ClientModelPropertiesManager(ClientModel model, JavaSettings settings) {
xmlRootElementNamespace = model.getXmlNamespace();
}

Set<String> thisModelPropertySerializeNames = model.getProperties()
.stream()
Set<String> thisModelPropertySerializeNames = Stream.concat(
// discriminator property is known to be redefined in subclass
.filter(property -> !property.isPolymorphicDiscriminator())
model.getProperties().stream().filter(property -> !property.isPolymorphicDiscriminator()),
// Canonicalized parent discriminators mask inherited ordinary properties with the same wire name.
model.getParentPolymorphicDiscriminators().stream())
Comment thread
XiaofeiCao marked this conversation as resolved.
.map(ClientModelProperty::getSerializedName)
.filter(name -> Objects.nonNull(name) && !name.isEmpty())
.collect(Collectors.toSet());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import com.microsoft.typespec.http.client.generator.core.model.javamodel.JavaFile;
import com.microsoft.typespec.http.client.generator.core.model.javamodel.JavaVisibility;
import com.microsoft.typespec.http.client.generator.core.util.ClientModelUtil;
import java.util.Objects;
import java.util.function.Consumer;
import java.util.function.Function;

Expand Down Expand Up @@ -116,7 +117,13 @@ private static void declareFieldInternal(ClientModelProperty discriminator, Clie
&& settings.isShareJsonSerializableCode()) {
classBlock.memberVariable(JavaVisibility.PackagePrivate, fieldSignature);
} else if (!allPolymorphicModelsInSamePackage || !settings.isShareJsonSerializableCode()) {
classBlock.privateMemberVariable(fieldSignature);
// Fixed inherited parent discriminators are final; active discriminators remain mutable for fallback.
if (discriminator.isConstant()
&& !Objects.equals(discriminator.getSerializedName(), model.getPolymorphicDiscriminatorName())) {
classBlock.privateFinalMemberVariable(fieldSignature);
} else {
classBlock.privateMemberVariable(fieldSignature);
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.util.Collection;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.ListIterator;
import java.util.Objects;
import java.util.Set;
import java.util.function.Function;
Expand Down Expand Up @@ -355,19 +356,15 @@ public ClientModel map(ObjectSchema compositeType) {
result = builder.build();

if (isPolymorphic && !CoreUtils.isNullOrEmpty(derivedTypes)) {
// Walk the polymorphic hierarchy finding places where the parent model and child model have different
// polymorphic discriminators. When this case is found add the parent polymorphic discriminator as a
// parent
// polymorphic discriminator to the child model. This is necessary to ensure that the child model
// generates
// the correct serialization in multi-level polymorphic structures.
// Preserve the fixed outer discriminator when a child starts a nested discriminator hierarchy.
for (ClientModel derivedType : derivedTypes) {
if (!Objects.equals(polymorphicDiscriminator, derivedType.getPolymorphicDiscriminatorName())) {
ClientModelProperty parentDiscriminator = result.getPolymorphicDiscriminator()
.newBuilder()
.defaultValue(result.getPolymorphicDiscriminator()
.getClientType()
.defaultValueExpression(derivedType.getSerializedName()))
.constant(true)
Comment thread
XiaofeiCao marked this conversation as resolved.
.build();

passPolymorphicDiscriminatorToChildren(parentDiscriminator, derivedType);
Expand All @@ -381,14 +378,60 @@ public ClientModel map(ObjectSchema compositeType) {
return result;
}

/**
* Propagates a fixed discriminator from an outer hierarchy through a nested discriminator hierarchy.
* <p>
* The {@code parentDiscriminator} is the fixed outer selection, while {@code child}'s discriminator remains the
* active discriminator for its own descendants. For example, given an outer {@code type} discriminator, a
* {@code type="message"} child that introduces {@code role}, and a {@code role="assistant"} grandchild, both nested
* models retain the canonical {@code type="message"} value while {@code role} controls nested dispatch.
* <p>
* A child can also declare an ordinary fixed property with the same wire name as the propagated discriminator.
* Keeping both representations would generate duplicate fields and accessors. Such a property must match
* {@code parentDiscriminator}'s Java name, wire type, client type, and fixed value, and must be constant; otherwise
* mapping fails. A valid property is removed from {@link ClientModel#getProperties()}, leaving
* {@code parentDiscriminator} as the canonical entry in
* {@link ClientModel#getParentPolymorphicDiscriminators()}.
* <p>
* Parent models map after their children, so the canonical entry is inserted at index zero to retain
* outer-to-inner discriminator order. The fixed discriminator is then recursively propagated to every descendant.
* For current stream-style JSON, same-package hierarchies may serialize this metadata through shared
* {@code toJsonShared} code. When hierarchy models are in different packages, or sharing is disabled, each model
* serializes inherited discriminator metadata through {@code serializeParentJsonProperties}, which consumes
* {@link ClientModel#getParentPolymorphicDiscriminators()}.
*
* @param parentDiscriminator the fixed discriminator selected by the outer hierarchy
* @param child the nested-hierarchy model that receives the fixed discriminator
* @throws IllegalStateException if the child declares the same wire name without matching constant status, Java
* name, wire type, client type, and fixed value
*/
private static void passPolymorphicDiscriminatorToChildren(ClientModelProperty parentDiscriminator,
ClientModel child) {
// Due to the execution order of ModelMapper, where children models complete mapping before the parent model,
// the parent polymorphic discriminator needs to be added at index 0. Reason, given an example where there are
// three models, where model #1 is the root parent with discriminator type, model #2 is a child of model #2 with
// discriminator kind, and model #3 is a child of model #3 with discriminator form. The order if this running
// will have model #2 add its discriminator to model #3 before model #1 runs adding its discriminator to #2 and
// #3. We want #3 to have the ordering of [type, kind], to represent the ordering of the parent models.
ListIterator<ClientModelProperty> iterator = child.getProperties().listIterator();
while (iterator.hasNext()) {
ClientModelProperty childProperty = iterator.next();
if (!Objects.equals(parentDiscriminator.getSerializedName(), childProperty.getSerializedName())) {
continue;
}

if (!childProperty.isConstant()
|| !Objects.equals(parentDiscriminator.getName(), childProperty.getName())
|| !Objects.equals(parentDiscriminator.getWireType(), childProperty.getWireType())
|| !Objects.equals(parentDiscriminator.getClientType(), childProperty.getClientType())
|| !Objects.equals(parentDiscriminator.getDefaultValue(), childProperty.getDefaultValue())) {
Comment thread
XiaofeiCao marked this conversation as resolved.
throw new IllegalStateException("Property '" + childProperty.getSerializedName() + "' on model '"
+ child.getName() + "' does not match its inherited polymorphic discriminator. Expected (name="
+ parentDiscriminator.getName() + ", constant=true, type=" + parentDiscriminator.getClientType()
+ ", value=" + String.valueOf(parentDiscriminator.getDefaultValue()) + "), but found (name="
+ childProperty.getName() + ", constant=" + childProperty.isConstant() + ", type="
+ childProperty.getClientType() + ", value=" + String.valueOf(childProperty.getDefaultValue())
+ ").");
}

iterator.remove();
break;
}

child.getParentPolymorphicDiscriminators().add(0, parentDiscriminator);

for (ClientModel derived : child.getDerivedModels()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -873,31 +873,19 @@ private void addModelConstructor(ClientModel model, ClientModelPropertiesManager

superProperties.append(property.getName());
} else {
/*
* here because the property in superclass constructor is overwritten in this model
* one example is
*
* model ParentModel {
* property: string;
* }
* model Model extends ParentModel {
* property: "constant";
* }
*
* we use the property in this model to initiate the superclass
*/
ClientModelProperty propertyInThisModel = model.getProperties()
.stream()
// Canonicalized discriminators can supply a fixed inherited constructor argument.
ClientModelProperty overridingProperty = Stream
.concat(model.getProperties().stream(), model.getParentPolymorphicDiscriminators().stream())
.filter(p -> Objects.equals(p.getSerializedName(), property.getSerializedName()))
.findFirst()
.orElse(null);
if (propertyInThisModel != null) {
if (propertyInThisModel.isConstant() && !property.isConstant()) {
if (overridingProperty != null) {
if (overridingProperty.isConstant() && !property.isConstant()) {
// property changed to constant in this model, use constant value to initiate super
// class
superProperties.append(propertyInThisModel.getDefaultValue());
superProperties.append(overridingProperty.getDefaultValue());
} else {
superProperties.append(propertyInThisModel.getName());
superProperties.append(overridingProperty.getName());
}
} else {
// this should not happen
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2059,8 +2059,16 @@ private void writeToXml(JavaClass classBlock) {
+ propertiesManager.getXmlNamespaceConstant(namespace) + ");"));

// Assumption for XML is polymorphic discriminators are attributes.
if (propertiesManager.getDiscriminatorProperty() != null) {
serializeXml(methodBlock, propertiesManager.getDiscriminatorProperty().getProperty(), false);
ClientModelPropertyWithMetadata discriminatorProperty
= propertiesManager.getDiscriminatorProperty();
model.getParentPolymorphicDiscriminators()
.stream()
.filter(discriminator -> discriminatorProperty == null
|| !Objects.equals(discriminator.getSerializedName(),
discriminatorProperty.getProperty().getSerializedName()))
.forEach(discriminator -> serializeXml(methodBlock, discriminator, false));
if (discriminatorProperty != null) {
serializeXml(methodBlock, discriminatorProperty.getProperty(), false);
Comment thread
XiaofeiCao marked this conversation as resolved.
}

propertiesManager.forEachSuperXmlAttribute(property -> serializeXml(methodBlock, property, true));
Expand Down Expand Up @@ -2214,7 +2222,7 @@ private void writeSuperTypeFromXml(JavaClass classBlock) {
+ propertiesManager.getXmlNamespaceConstant(discriminatorProperty.getXmlNamespace()) + ", "
+ "\"" + discriminatorProperty.getSerializedName() + "\");");
} else {
methodBlock.line("String discriminatorValue = reader.getStringAttribute(" + "\""
methodBlock.line("String discriminatorValue = reader.getStringAttribute(null, " + "\""
+ discriminatorProperty.getSerializedName() + "\");");
}

Expand All @@ -2226,12 +2234,18 @@ private void writeSuperTypeFromXml(JavaClass classBlock) {
// Add deserialization for all child types.
List<ClientModel> childTypes = getAllChildTypes(model, new ArrayList<>());
for (ClientModel childType : childTypes) {
boolean sameDiscriminator = Objects.equals(childType.getPolymorphicDiscriminatorName(),
model.getPolymorphicDiscriminatorName());
if (!sameDiscriminator && !Objects.equals(childType.getParentModelName(), model.getName())) {
continue;
}

String deserializationMethod = (isSuperTypeWithDiscriminator(childType) && sameDiscriminator)
? ".fromXmlInternal(reader, finalRootElementName)"
: ".fromXml(reader, finalRootElementName)";
ifBlock = ifOrElseIf(methodBlock, ifBlock,
"\"" + childType.getSerializedName() + "\".equals(discriminatorValue)",
ifStatement -> ifStatement
.methodReturn(childType.getName() + (isSuperTypeWithDiscriminator(childType)
? ".fromXmlInternal(reader, finalRootElementName)"
: ".fromXml(reader, finalRootElementName)")));
ifStatement -> ifStatement.methodReturn(childType.getName() + deserializationMethod));
}

if (ifBlock == null) {
Expand Down Expand Up @@ -2439,6 +2453,10 @@ private void writeFromXmlDeserialization(JavaBlock methodBlock) {
}

private void deserializeXmlAttribute(JavaBlock methodBlock, ClientModelProperty attribute, boolean fromSuper) {
if (attribute.isRequired() && attribute.isConstant() && !attribute.isPolymorphicDiscriminator()) {
return;
}

String xmlAttributeDeserialization = getSimpleXmlDeserialization(attribute.getWireType(), null,
attribute.getXmlName(), propertiesManager.getXmlNamespaceConstant(attribute.getXmlNamespace()), true);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public final class GoblinShark extends Shark {
/*
* Discriminator property for Fish.
*/
private String kind = "shark";
private final String kind = "shark";

/*
* The sharktype property.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public final class SawShark extends Shark {
/*
* Discriminator property for Fish.
*/
private String kind = "shark";
private final String kind = "shark";

/*
* The sharktype property.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public class Shark extends FishInner {
/*
* Discriminator property for Fish.
*/
private String kind = "shark";
private final String kind = "shark";

/*
* The sharktype property.
Expand Down
Loading
Loading