Fix issue with Discriminator type resolution that impacted the decoding of oneOf types in some conditions

[liamnichols]

Background

The oneOf OpenAPI schema types are represented as enum types in generated Swift code where each nested type becomes an enum case. Each enum case is named after the type that it represents and with this, there is one enum case per type.

When decoding, a discriminator, which defines a property used to disambiguate, is used to help decode the right payload into the right type. The init(from:) decoder will switch on the discriminator value and try to decode the container into the type that matches the appropriate case.

We identified a problem however where in some scenarios, the case doesn't get added to the switch in the decoder implementation which results in the decoding failing.

I reproduced this in the following commit: 35d48de. In the diff, you will notice that the discriminator lists a mapping with values one, two and three however the decodable implementation only switches on a value of three. one and two are missed from the generated code.

Note:

This happens in this particular example because the One and Two types are identical to their reference A so CreateAPI has optimised things by skipping them. You will notice that the enum itself has spotted that since there is an a case and a three case, but the decodable init didn't figure this out.

Changes

After some digging, I traced this back to the way that the Discriminator type is build when evaluating the schema:

CreateAPI/Sources/CreateAPI/Generator/Generator+Schemas.swift

Lines 402 to 428 in 5a6a200

	private func makeDiscriminator(info: JSONSchemaContext, context: Context) throws -> Discriminator? {
	if let discriminator = info.discriminator {
	var mapping: [String: TypeIdentifier] = [:]
	if let innerMapping = discriminator.mapping {
	for (key, value) in innerMapping {
	let stripped = String(value.dropFirst("#/components/schemas/".count))
	guard let componentKey = OpenAPI.ComponentKey(rawValue: stripped) else {
	throw GeneratorError("Encountered invalid type name \"\(value)\" while constructing discriminator")
	}
	if let name = getTypeName(for: componentKey) {
	mapping[key] = .userDefined(name: name)
	} else {
	try handle(warning: "Mapping \"\(key)\" has no matching type")
	}
	}
	}
	return .init(
	propertyName: discriminator.propertyName,
	mapping: mapping
	)
	}
	return .none
	}

In the prior implementation, it would try and resolve the ref string that was provided as a mapping to a type. In this case, it resolved it as One and Two, but later on, since those types didn't get generated, it just skipped them in the initialiser body. The problem here is that this implementation differs to how we resolve names of references elsewhere so the logic for optimising out the redundant One and Two is missed and the bug is reproduced.

To fix the issue, it's a relatively straightforward case of using the existing getReferenceType(_:context:) method that is used elsewhere instead. After making the change and regenerating snapshots, we get the correct result (35d48de).

[liamnichols]

This roughly matches the implementation in OpenAPIKit but I didn't pay too much attention to external references since we don't actually support them currently.

[imjn]

Thank you very much for fixing this 🙏🏽
Looks great to me.

Now we may remove these 2 workarounds which are likely related to what you fixed...?

CreateAPI/Sources/CreateAPI/Generator/Generator+Schemas.swift

Lines 448 to 452 in 2953e7c

	// Covers a weird case encountered in open-banking.yaml spec (xml-sct schema)
	// TODO: We can potentially inline this instead of creating a typealias
	if entity.properties.count == 1, entity.properties[0].nested == nil {
	return TypealiasDeclaration(name: name, type: entity.properties[0].type, nested: entity.properties[0].nested)
	}

CreateAPI/Sources/CreateAPI/Generator/Generator+Schemas.swift

Lines 494 to 503 in 2953e7c

	// TODO: Improve this and adopt for other types (see Zoom spec)
	if properties.count == 1 {
	var property = properties[0]
	if let nested = property.nested as? EntityDeclaration, nested.name.rawValue == "Object" {
	nested.name = name
	property.nested = nested
	property.type = .userDefined(name: name)
	}
	return TypealiasDeclaration(name: name, type: property.type, nested: property.nested)
	}

[liamnichols]

Now we may remove these 2 workarounds which are likely related to what you fixed...?

Ahh great spot @imjn, I see how these are likely related now (to begin with, I struggled to understand what they might be doing 😅)

@kean By any chance do you remember any more details about the two workarounds above? Are you able to provide a reproducible example of them in use so that I can understand if my change solve them? Were they related to the Discriminator alone?

Removing these workarounds doesn't break anything in the CreateAPI test suite so I might just do it and see if anything gets raised.

[kean]

I don't remember the details, but I suggest running before and after for the two attached specs and checking the diff: specs.zip. Discriminator is a new addition. I haven't worked on it.

[liamnichols]

Thanks for confirming, I ran those schemas against this change before and after and it makes no difference. I also tested removing those workarounds and while the Zoom one seems to be redundant now, the OpenBanking one appears to still be relevant.

I'll leave it for now but will look into that later on at some point 👍