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

Provide @AlwaysEncode / @EncodeDefault annotation #1091

Closed
sandwwraith opened this issue Sep 21, 2020 · 19 comments
Closed

Provide @AlwaysEncode / @EncodeDefault annotation #1091

sandwwraith opened this issue Sep 21, 2020 · 19 comments
Assignees

Comments

@sandwwraith
Copy link
Member

What is your use-case and why do you need this feature?

kotlinx.serialization framework allows to drop properties from output when its values are equal to default ones (see encodeDefault flag). However, this is configurable per format and not per property. Sometimes, it may be convenient to have a default value in Kotlin class, but it also may be necessary to always serialize this value to JSON — for example, if this is a protocol/api version for some app.

See also: #58 (comment)

Describe the solution you'd like

Annotation with property target that allows to force encoding even if property value is equal to the default one.

@ArcticLampyrid
Copy link

For null-not-supported formats like ProtoBuf, it is needed to encode nullable value with EncodeDefault = false.
However, we may need to encode other default values like foo : Int = 0.
In this way, config per field is useful.

@ArcticLampyrid
Copy link

Describe the solution you'd like

Add a enum and a annotation

public enum EncodeDefaultMode{
    DEFAULT,
    ALWAYS,
    NEVER
}

@Target(AnnotationTarget.PROPERTY)
public annotation class EncodeDefault(public val mode: EncodeDefaultMode = EncodeDefaultMode.DEFAULT)

Then:

  • Change the compiler plugin, when EncodeDefaultMode is set to ALWAYS or NEVER, do not call shouldEncodeElementDefault but using a constant (high-performance, in my favor)
  • OR change shouldEncodeElementDefault to check EncodeDefaultMode at runtime (low-performance, but more maintainable)

@sandwwraith
Copy link
Member Author

Can you, please, explain, why there is a need in this enum values (especially NEVER)? Original issue was just about @AlwaysEncode annotation without parameters. NEVER, judging by your PRs, simply checks equality with the default value and does not respect format settings, which may confuse users.

Also, default value of annotation parameter (DEFAULT) seems dull: when @EncodeDefault annotation is used with it, it does not change anything at all. Maybe DEFAULT shouldn't even be an option.

@ArcticLampyrid
Copy link

NEVER does not respect format settings

If you think so, ALWAYS does not "respect" format settings too, since we set encodeDefaults=false but the field will also be encoded.

Why NEVER is needed.

To provide a more precise control of optionality (per field), of course.
NEVER can be replaced by setting encodeDefaults=false and marking all the other fields by ALWAYS. But it can be more convenient in some situation.

Maybe DEFAULT shouldn't even be an option

Well, DEFAULT can be removed, it's just a problem about design style.

@ArcticLampyrid
Copy link

One usage case about NEVER:
ProtoBuf does not have the concept about NULL, but many library use "omitted field" to represent NULL.
For compatibility with other libraries, omitting nothing but null values is need.
We may want to drop null values in ProtoBuf (#588), but we still want to encode the defaults.
At present, kotlinx.serialization uses default values to handle NULL values to be dropped (#651). For me, marking nullable field with NEVER (and settingencodeDefaults=true) is more convenient than marking all the other fields with ALWAYS (and settingencodeDefaults=false).

@ArcticLampyrid
Copy link

In this PR, @EncodeDefault is designed to overwrite the format's settings for specific fields.
And NEVER means encodeDefault=false, ALWAYS means encodeDefault=true.

It can be also designed as @EncodeDefault(true) / @EncodeDefault(false). IMO, enum is more semantic.

@bartvanheukelom
Copy link

I'm in favour of this feature request, but I'd make 2 annotations for simplicity, @EncodeDefault and @NoEncodeDefault.

I agree with @sandwwraith that the DEFAULT option doesn't add value here (besides having a confusing name).
Now, where such a thing could have value is in a feature that allows overriding format options for entire object trees, where it could undo that override. For example:

@EncodeDefaults(NEVER)
class Root

@EncodeDefaults(CONTEXTUAL) // by format settings
class Branch

@EncodeDefaults(ALWAYS)
class Leaf

But that's unrelated to this feature request, I think.

@ArcticLampyrid
Copy link

I'd make 2 annotations for simplicity

It is indeed simplified. But enum will indicate that ALWAYS and NEVER can't be used together and that there are only two (or three, if DEFAULT included) modes available, which is more semantic.

So we may have to investigate further to make a choice.

@valeriyo
Copy link

valeriyo commented Feb 8, 2021

The existing annotation kotlinx.serialization.Required appears to work as the proposed @AlwaysEncode. Tested with Koitlin 1.4.30 and serialization 1.0.1.

The doc for @Required says:

 > Indicates that property must be present during deserialization process, despite having a default value.

which sounds a bit misleading, because it affects encoding (not only decoding).

@ArcticLampyrid
Copy link

@AlwaysEncode is designed to affect encoding (not decoding) only.
For fields with default values, when encoding, it can be omitted or not. @AlwaysEncode is proposed to decide whether it is omitted.

However, @Required is designed to tell that the field is not optional on serialization level, even if there are default values on Kotlin level.

@valeriyo
Copy link

valeriyo commented Feb 8, 2021

@ArcticLampyrid sorry, could you explain again? 😕

What I see is that with Json { encodeDefaults = false }:

  • val operation = "create" is omitted from JSON, but
  • @Required val operation = "create" outputs operation: "create"

Isn't this what the proposed @AlwaysEncode is supposed to do? What's different?

@ArcticLampyrid
Copy link

While decoding, {} will fail with @Required because operation is missing. But with @AlwaysEncode it will be okey since operation is still optional.

@AlwaysEncode does not prevent a field from being optional, it just forces the writer to write the default value.

@valeriyo
Copy link

valeriyo commented Feb 8, 2021

@ArcticLampyrid aaah. makes sense now, thanks for the explanation 👍

@ArcticLampyrid
Copy link

Control EncodeDefault per field is supported in some famous serialization libraries.
And most of them has EncodeDefault both in the field level and the serialization settings level, so I don't think this annotation confusing.

Library Serialization Settings Level Field Level
(.NET) Newtonsoft.Json JsonSerializerSettings.DefaultValueHandling JsonPropertyAttribute.DefaultValueHandling
(.NET) System.Text.Json JsonSerializerOptions.DefaultIgnoreCondition JsonIgnoreAttribute
(Rust) serde - skip_serializing meta programming expression (very flexible)
(Java) Jackson ObjectMapper.setDefaultPropertyInclusion JsonInclude Annotation

@ArcticLampyrid
Copy link

It can be also designed as @EncodeDefault(true) / @EncodeDefault(false). IMO, enum is more semantic.

For further:
EncodeDefaultMode.NOT_EMPTY_LIST or even @EncodeDefault(DefaultValueDetector::class)

@sandwwraith
Copy link
Member Author

Thanks for the study; I'd still favor the argument-less @EncodeDefault, but we'll revisit this design and see if we need a NEVER option

Regarding Protobuf, null handling is a completely different question, that is under implementation now

@Dominaezzz
Copy link
Contributor

I wonder how this is supposed to interact with formats that mandate that default values are not encoded. DER (a strict subset of BER) is an example of such format (which I happen to be implementing). It's important for signing to make sure all DER encoders generate the exact same DER encoded object so signatures are consistent.

@ArcticLampyrid
Copy link

@Dominaezzz
Then you should not use ALWAYS, but it will be okey to mark the field with NEVER or DEFAULT.
Additional safety check can be added, but it's not urgent.
No matter what the format is, you need to define the format as required. You need to use the same names, values as those of other encoders, so does EncodeDefaultMode.

@ArcticLampyrid
Copy link

BTW, in your example of DER format, it seems that DER encoder support all modes (ALWAYS/NEVER), but in your case the protocol is defined that all the field should work with NEVER mode.

It' similar that we can omit all defaults in JSON while the client assume that we never omit any defaults, so we use ALWAYS mode for cooperativity. (more relating to protocol than format)

dariuszkuc pushed a commit to dariuszkuc/graphql-kotlin that referenced this issue May 25, 2021
Wrap optional input arguments in new `OptionalInput` sealed class which can either be `Undefined` (omitted) or `Defined` (value or explicit null). Unfortunately it looks like we won't be able to support this functionality for `kotlinx-serialization` until Kotlin/kotlinx.serialization#1091 is resolved.

Related: ExpediaGroup#1151
smyrick pushed a commit to ExpediaGroup/graphql-kotlin that referenced this issue May 26, 2021
* [client] support optional input for jackson serializer

Wrap optional input arguments in new `OptionalInput` sealed class which can either be `Undefined` (omitted) or `Defined` (value or explicit null). Unfortunately it looks like we won't be able to support this functionality for `kotlinx-serialization` until Kotlin/kotlinx.serialization#1091 is resolved.

Related: #1151

* fix build

* add explicit opt-in flag for plugins to use optional input wrapper
dariuszkuc added a commit to dariuszkuc/graphql-kotlin that referenced this issue Aug 5, 2022
…1158)

* [client] support optional input for jackson serializer

Wrap optional input arguments in new `OptionalInput` sealed class which can either be `Undefined` (omitted) or `Defined` (value or explicit null). Unfortunately it looks like we won't be able to support this functionality for `kotlinx-serialization` until Kotlin/kotlinx.serialization#1091 is resolved.

Related: ExpediaGroup#1151

* fix build

* add explicit opt-in flag for plugins to use optional input wrapper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants