Backwards compatibility issue starting version 0.3.1

[augustinasrce]

0.3.2 kotlin-sdk - 0.1.1 goff-provider seems to be incompatible, so it should be marked as breaking changes.

[samuolis]

Hey,

I will add a bit more details to the issue report. With the release of 0.3.1 the library had an update with changes to ProviderEvaluation data class, that is used by other libraries, in our example by GoLang provider in this place. It generates ProviderEvaluation data class. As this provider was only updated after a few weeks, we had a new version of Open-feature of 0.3.2 and an older version of goff-provider 0.1.1 (the provider was still using the 0.3.0 version of open-feature). This caused errors inside evaluation of the flag:

java.lang.NoSuchMethodError: No direct method <init>(Ljava/lang/Object;Ljava/lang/String;Ljava/lang/String;Ldev/openfeature/sdk/exceptions/ErrorCode;Ljava/lang/String;)V in class Ldev/openfeature/sdk/ProviderEvaluation; or its super classes (declaration of 'dev.openfeature.sdk.ProviderEvaluation' appears in base.apk!classes4.dex)

Few suggestions:
Maybe it would still be good to create two constructors for ProviderEvaluation for backwards compatibility. The new one would just be the same 5-param constructor without metadata and would just add metadata as empty one and other as it is right now. Something similar to this one:

data class ProviderEvaluation<T>(
    val value: T,
    val variant: String? = null,
    val reason: String? = null,
    val errorCode: ErrorCode? = null,
    val errorMessage: String? = null,
    val metadata: EvaluationMetadata = EvaluationMetadata.EMPTY
) {
    constructor(
        value: T,
        variant: String? = null,
        reason: String? = null,
        errorCode: ErrorCode? = null,
        errorMessage: String? = null
    ) : this(value, variant, reason, errorCode, errorMessage, EvaluationMetadata.EMPTY)
}

Or it would be good to update release notes for version 0.3.1 that it include breaking changes in ProviderEvaluation and providers must be up to date.

[nicklasl]

Hello!
I am not doubting that the change caused issues and I appreciate the feedback!
I'll update the release notes for that version to include a note about this issue.

As far as remediations however, the ProviderEvaluation data class should already now have multiple constructors, like the one you suggest (and will work even better with named arguments).

All of below are valid ways to create an instance:

val evaluation1 = ProviderEvaluation(true)
val evaluation2 = ProviderEvaluation(true, "variant")
val evaluation3 = ProviderEvaluation(true, "variant", "reason")
val evaluation4 = ProviderEvaluation(true, "variant", "reason", ErrorCode.GENERAL)
val evaluation5 = ProviderEvaluation(true, "variant", "reason", ErrorCode.GENERAL, "message")
val evaluation6 = ProviderEvaluation(true, "variant", "reason", ErrorCode.GENERAL, "message", EvaluationMetadata.EMPTY)
val evaluation7 = ProviderEvaluation(
  value = true,
  variant = "variant",
  reason = "reason",
  metadata = EvaluationMetadata.EMPTY
)

So I'm not sure how another constructor would solve it but maybe I'm misunderstanding something.

[nicklasl]

Please take a look at this PR if you think its a good mitigation:
#123

[samuolis]

Yes, the default params will be set, and that's fine, but the library was not able to find a constructor for 5 params, I don't know where the issue was fully then. You can try to test this out by creating a provider that would use the old version inside, without metadata and then use kotlinSDK with metadata; then, it will have a similar crash to this one. And yes you are right it should be compiled by overloading constructor and the same should have happened, but in reality provider with lower version was not able to construct ProviderEvaluation and was throwing errors, that was really strange

[nicklasl]

I'm even more confused now. The bump from OF 0.3.0 to 0.3.2 in GoLang provider was done in this PR thomaspoignant/go-feature-flag#2839 which I assume would have needed to compile before being merged?

So is it a runtime issue?

[samuolis]

Yes it was a runtime issue, it only happened when there was a flag to evaluate, if there is no flag request from the library, the exception were not happening