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

Add @Default annotation to optionally mark dynamically initialized attibutes with constand value #1345

Closed
ComBatVision opened this issue Feb 17, 2021 · 12 comments
Labels

Comments

@ComBatVision
Copy link

ComBatVision commented Feb 17, 2021

Very strange behavior of Protobuf serialization found.
It always works perfect in debug session, but when I execute the same single thread code in usual run mode and repeat the same serialization in loop it fails to serialize one field from the class approx. 70% of time, but works other 30% times.

To Reproduce
There are parent abstract class and its child class like this:

@ExperimentalSerializationApi
@Serializable
abstract class IdentifiableModel {
    @ProtoNumber(1) abstract val uri: String
    @ProtoNumber(2) var startDate = DateTime.now().unixMillisLong
    @ProtoNumber(3) var endDate: Long? = null
    @ProtoNumber(4) var reportingDate = DateTime.now().unixMillisLong
}

@ExperimentalSerializationApi
@Serializable
@SerialName("Thing")
open class ThingModel(
    @ProtoNumber(1) override val uri: String,
): IdentitfiableModel() {
    @ProtoNumber(10) var name = ""
    @ProtoNumber(12) var additionalInformationText = ""
}

@ExperimentalSerializationApi
val serializersModule = SerializersModule {
    polymorphic(IdentifiableModel::class) {
        subclass(ThingModel::class)
    }
}

val proto = ProtoBuf( serializersModule = module )

I have created usual Proto2 declaration and generate Java classes with protoc to test serialization like this:

syntax = "proto2";
package vision.combat.c4.node.evc.proto;
option java_outer_classname="ProtoEntity";

message DataWrapper {
  required string type = 1;
  required Data value = 2;
}

message Data {
  required string uri = 1;
  required int64 startDate = 2;
  optional int64 endDate = 3;
  required int64 reportingDate = 4;
}

When I execute following code, I receive error - "InvalidProtocolBufferException: Message missing required fields: value.reportingDate"

val thing = ThingModel("test")
val serialized = proto.encodeToByteArray(IdentifiableModel.serializer(), thing)
val unserialized = ProtoEntity.DataWrapper.parseFrom(serialized); // Standard Google class do not see reportingDate

I have logged serialization process, and I found that reportingDate is always present in thing object before serialization, but the length of resulting serialized byte array is sometimes 106, and sometimes 113 bytes during the same runtime, just different loop steps.

When I run the same code in debug, serialized length is always 113 and there is no error.
Error is always related to Reporting date field.

Expected behavior
It should de-serialize byte array with Google Proto2 class all times, not only in debug mode. Or at least byte array length should keep the same.

Environment

  • Kotlin version: 1.4.30
  • Library version: 1.0.1 and 1.1.0-RC
  • Kotlin platforms: JVM
@shanshin
Copy link
Contributor

Be careful with using property default values especially DateTime.now().
By default ProtoBuf format not encode default values so using now() gives an unstable result because serialization time may be less than one millisecond.

For your case, you may try to declare a format like this

val proto = ProtoBuf { encodeDefaults = true; serializersModule = yourModule }

@ComBatVision
Copy link
Author

ComBatVision commented Feb 18, 2021

Thank you @shanshin,
Now I understand why it fails.
I will redesign models to have 0 as constant default value and will initialize them in init block.


@ExperimentalSerializationApi
@Serializable
abstract class IdentifiableModel {
    @ProtoNumber(1) abstract val uri: String
    @ProtoNumber(2) @Required var startDate = 0L
    @ProtoNumber(3) var endDate: Long? = null
    @ProtoNumber(4) @Required var reportingDate = 0L

    init {
        // Initialize dates separately to avoid default values serialization problem
        val now = DateTime.now().unixMillisLong
        startDate = now
        reportingDate = now
    }
}

So it is probably not a bug of a library, but its normal behavior.
But it was very hard to predict this for me :)

I will tell if it fix my issue.

@ComBatVision
Copy link
Author

ComBatVision commented Feb 18, 2021

My approach solve de-serialization issue, but Init block executes after values mapping from Proto and overrides de-serializer values.
Is it ok or it is a bug?
I thought calss constructor should executes before applying de-serialized values. But it is not.

Workaround will be to override required properties only if they have no default value, because Init block executes after de-serialization.

    init {
        // Initialize required dates separately to avoid default values serialization problem
        val now = DateTime.now().unixMillisLong
        if (startDate == 0L) {
            startDate = now
        }
        if (reportingDate == 0L) {
            reportingDate = now
        }
    } 

@ComBatVision
Copy link
Author

I propose you to add @default(<constant_value>) annotation to be able to override default behavior and allow developers to use dynamically defined default values in a class.

@ComBatVision ComBatVision changed the title Protobuff serialization works in debug mode, but skip one specific field during usual run Add @Default annotation to optionally mark dynamically initialized attibutes with constand value Feb 18, 2021
@shanshin
Copy link
Contributor

@ComBatVision yes, init block can override deserialized value.

I thought calss constructor should executes before applying de-serialized values. But it is not. constructor and init blocks needs to see deserialized value for consistency of transient properties and side-effects.

@Default annotation look excessively, a compile-time constant default value can be written in the initializer. @Default(123) val i: Int and val i: int = 123 are the same.

@ComBatVision
Copy link
Author

@default annotation looks excessively in standard situation, when it is not required, but it can help to solve issue with dynamic default values:
@Default(0) var attr = initValue()

My case with DateTime.now() or any other case where attribute receive dynamic default value will face current issue.

@shanshin
Copy link
Contributor

init block applicable to these cases. Compile-time constant does not cover all situations.
Also when you see property defined like @Default(10) var i: Int = 11 what value of i should be excluded while serialization with encodeDefaults=false?

@ComBatVision
Copy link
Author

ComBatVision commented Feb 19, 2021

@default(10) should force serializer to ignore any value after =. In your example it should skip 10 when encodeDefaults=false.

Its usage will be similar to @required annotation. It is also useless for val declaration but used for var cases with default values.

@shanshin
Copy link
Contributor

For difficult cases, you can define a custom serializer and implement the required behavior.
@Requered @Default(0) var i: Int = 11 for symmetry should work so: if i value is 11 then serialize it as 0 - it will be even less obvious to users.
Additionally Default annotation has a number of limitations: it works only with compile-time constants, it cant work with ranges and complex conditions - this greatly narrows its scope, but makes the code unobvious.
For difficult custom logic preferable to use init blocks or custom serializers.

@ComBatVision
Copy link
Author

ComBatVision commented Feb 19, 2021

Ok. As you wish. It was just my proposition to minimize code.

I have solved my initial issue by using init block.
But i thought that Default annotation will be more simple way for dynamically defined properties.

I did not proposed to make Default annotation obligatory. I just propose it to be used in case you want to force serializer to ignore usual default value in attribute.

@shanshin
Copy link
Contributor

shanshin commented Feb 19, 2021

Existing encodeDefault a mechanism exists to support the ability to set property values if the attribute was not passed.
For example in JSON for class @Serializable data class Holder(val a: Int, val b: Int = 10) and in source JSON attribute b not passed - while deserialization it will take the value 10 (e.g. JSON {"a": 1} instance will deserialized as Holder(a=1, b=10)).
In your case, you use a different approach in the form of substitution of the passed values. Mixing these approaches may not be obvious to users and does not accurately cover common cases of value substitution.

@sandwwraith
Copy link
Member

Also see the related discussion here: #1321 . We do not intend to implement any kind of @Default annotation, instead, we propose @EncodeDefault: #1091 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants