Enhance support for default values

[aaronsky]

Fixes #169.

Breaking changes:

• Booleans are no longer treated as special amongst other properties in the spec. Therefore, if a boolean property is not declared as a required property, it may now render as an optional boolean.

Other changes:

• Properties with default values have their default values assigned in initializers and decoders, if their type is a specifically-supported builtin (previously this only worked for booleans).
• Properties with default values that match an enum case will use the enum case as default rather than nil.
• Required properties with enum types that contain only one case now automatically use the only case as a default value.

[liamnichols]

Wow big thank you for putting this Pull Request up so quickly! I've left an inline comment about the approach we are taking for the initialiser as I think there are a couple of other things we need to consider.

I also want to talk more about using the default value in the decodable implementation because with my understanding of a JSON schema/Open API spec, I don't think that the default keyword was defined for this.

The OpenAPI Docs don't say much about default, but they do say it's aligned with JSON Schema's definition, which states the following:

The default keyword specifies a default value. This value is not used to fill in missing values during the validation process. Non-validation tools such as documentation generators or form generators may use this value to give hints to users about how to use a value. However, default is typically used to express that if a value is missing, then the value is semantically the same as if the value was present with the default value. The value of default should validate against the schema in which it resides, but that isn’t required.

I can see the default value useful as a hit in the generated initialisers, but manipulating the decoded response to fill gaps with it feels like it might be a step too far since nothing suggests that nullable and default are related to each other (thinking outside of Swift).

That being said, I don't know everything so I could be wrong.. Have you compared how some of the other generators in existence use default? I might be swayed if there are other good examples but at the moment I'm leaning towards suggesting that we only apply the default to the initialisers. Let me know what you think, but thanks again for the work here!

[aaronsky]

I can see the default value useful as a hit in the generated initialisers, but manipulating the decoded response to fill gaps with it feels like it might be a step too far since nothing suggests that nullable and default are related to each other (thinking outside of Swift).

That being said, I don't know everything so I could be wrong.. Have you compared how some of the other generators in existence use default? I might be swayed if there are other good examples but at the moment I'm leaning towards suggesting that we only apply the default to the initialisers.

I can compare output to some of the other big generators. I generally agree that defaults should not be influencing the decision to generate decoder-inits. I'm also leaning towards minimizing the generation of decoder-inits as much as possible, but that would require us to have a little more insight into whether or not synthesis is possible.

[aaronsky]

I've put up a couple new solutions to the feedback you've given me. Let me know what you think of this once you've reviewed, given your time and energy.

[liamnichols]

@aaronsky sorry it took me so long to get back to this! Just to confirm from your last message, my understanding was that we aren't going to provide a default value when decoding via init(from:) and instead would only use the default values in the generated memberwise initialiser?

[liamnichols]

Are these changes right? I'm not sure what has caused them but we don't seem t have modified the schema have we?

[aaronsky]

My understanding of the OpenAPI spec, limited as it may be, is that it is technically more accurate because these properties are not required in their respective definitions.

[liamnichols]

Oh I see, this property should always have been optional because it is not included in the required array on the schema.

I guess the prior behaviour treated it as non-optional because it had a default value,. If that was the case, I agree that it was likely incorrect so am in favour of fixing it 👍

Do you think it would be possible to pull this bug fix out into its own Pull Request? I'm just being a bit cautious as its quite hard to follow amongst the other changes going on 🙇 It's also something that will likely cause a breaking change to people already using CreateAPI so should be called out as an independent fix in the release notes 🙏

[aaronsky]

I'm trying to unwind my changes since it's been a few months since I did most of this work, in order to give a definitive answer on this. It might be possible to split this out, but it's not immediately clear.

[aaronsky]

@aaronsky sorry it took me so long to get back to this! Just to confirm from your last message, my understanding was that we aren't going to provide a default value when decoding via init(from:) and instead would only use the default values in the generated memberwise initialiser?

I believe that's what we agreed on.

[liamnichols]

@aaronsky sorry it took me so long to get back to this! Just to confirm from your last message, my understanding was that we aren't going to provide a default value when decoding via init(from:) and instead would only use the default values in the generated memberwise initialiser?

I believe that's what we agreed on.

Ah ok then, I ask because I still see the nil coalescing to the default value in the snapshots, like this:

https://github.com/CreateAPI/CreateAPI/pull/170/files#diff-e17d0dd37de44a675de95aef390c1a217a05ce891f7bc9902e7b284b75ec5ed0R92

We shouldn't be doing that, right?

[aaronsky]

Actually that one is deliberate, because that is the default defined in the github.yml spec

[liamnichols]

I think that still goes against how we want to interpret default? We want to keep it to be provided as a hint for the memberwise initialiser but we don't really want to start making customisations to the decoder behaviour.

The property is still optional, so if the decoding container doesn't decode the value (either because it was omitted or because it was null?) then we should pass that on still rather than falling back to the default.

With the exception of the separate bug fix, it's important that the only change in generator output is the addition of a default value on the memberwise initialiser rather than behavioural changes to how decoding works. Maybe we could look into seeing if the decoder defaults becomes an opt-in change that we might want to support later on, but I don't think that it should happen by default or in this PR.

Does that sound ok? Thanks for sticking with me on this one as I also try to figure out how we want this to work 😅

[aaronsky]

@liamnichols I see what you mean now. I've pushed up a revision to (hopefully) address this.

[liamnichols]

I looked at the diff a bit closer and now I understand what is going on a bit more, but I am now also more confused 😄

CreateAPI was already using it's 'default' value in the decoder implementation but I didn't notice this because it only supported boolean defaults from the schema itself.

You can see this here:

CreateAPI/Tests/Support/Snapshots/edgecases-rename/Sources/Entities/Order.swift

Line 39 in da8730b

self.isComplete = try values.decodeIfPresent(Bool.self, forKey: "complete") ?? false

To be honest, this is a really confusing and unexpected behaviour (at least it is to me). If property is nullable/not required, we're still treating it as non-optional by overriding the Codable implementation to fallback to the default value.

It's kind of neat, but it's kind of also confusing and I am still not sure which direction we should take 😄

To me, this feels quite non-standard for a code generator to make that kind of decision. Would this in fact encourage poor schema design? Why is a property nullable in the first place if there is a default value?

Sorry to open a can of worms on this PR given that its existing behaviour, but I guess it's more important as we consider opening it up to more than just booleans.

[liamnichols]

Ahh so this is what fixes the bug in described in https://github.com/CreateAPI/CreateAPI/pull/170/files#r1047362572

[liamnichols]

Ahh but it does this because the generated decoder implementation was already using the defaultValue as a fallback 🙈

CreateAPI/Sources/CreateAPI/Generator/Templates.swift

Lines 244 to 245 in da8730b

	let defaultValue = (property.isOptional && property.defaultValue != nil) ? " ?? \(property.defaultValue!)" : ""
	return "self.\(property.name.accessor) = try values.\(decode)(\(property.type).self, forKey: \(key))\(defaultValue)"

Which is what we said that we should stop doing.

I guess this didn't show up much (at all?) before because it was limited to only booleans. Hmmm, now I'm confused as to what we should do because includeDefaultValues is true by default.