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

Set explicit default null values for nullable fields in generated schemas #294

Merged
merged 2 commits into from
Mar 29, 2021
Merged

Set explicit default null values for nullable fields in generated schemas #294

merged 2 commits into from
Mar 29, 2021

Conversation

gafiatulin
Copy link
Contributor

Some schema consumers require explicitly set "default": null for the "nullable union field".

Related: AVRO-1803

@grouzen
Copy link

grouzen commented Mar 23, 2021

Need this PR merged, please! :)

@bplommer
Copy link
Member

Sorry for the slow response - I'll try to take a look this evening.

@bplommer
Copy link
Member

I agree this behaviour should be available but I also think it should be optional - so keep the existing derive with its current behaviour but add something like deriveWithDefaultNulls. I'll have a go at adding that to the PR when I have a chance, unless you beat me to it 🙂

@gafiatulin
Copy link
Contributor Author

@bplommer thanks for looking into this.
I can change this to be annotation-enabled, something like @AvroDefaultNulls. I'll push something a bit later today.

@gafiatulin
Copy link
Contributor Author

@bplommer please see 7df614d.

@gafiatulin
Copy link
Contributor Author

gafiatulin commented Mar 25, 2021

I guess it's also possible to use magnolia's param default value extractor and have more generic annotation:

case class ABC(
  @AvroDefaultValue a: Int = 5,
  b: Option[String],
  @AvroDefaultValue c: Option[Long] = None 
)

=>

{
  "type" : "record",
  "name" : "ABC",
  "fields" : [ {
    "name" : "a",
    "type" : "int",
    "default" : 5
  }, {
    "name" : "b",
    "type" : [ "null", "string" ]
  }, {
    "name" : "c",
    "type" : [ "null", "long" ],
    "default" : null
  } ]
}

With implementation like:

...
param.annotations.collectFirst {
  case adv: AvroDefaultValue =>
    param.default match {
      case Some(defaultValue) =>
        param.typeclass.encode(defaultValue) match {
          case Left(err) => throw err.throwable
          case Right(value) =>
            // TODO: Handle other unions except for [null, ???]  
            if (schema.isNullable && value == null)
              Schema.Field.NULL_DEFAULT_VALUE
            else if (schema.isNullable)
              throw new Exception(
                s"For optional fields only supported default value is None, field ${param.label} has default value $defaultValue."
              )
            else value
        }
      case None =>
        throw new Exception(
          s"Missing default value for a field ${param.label} annotated with $adv."
        )
    }
}.orNull
...

Which kinda works, but because in avro default values for union fields have to correspond to the first schema in the union would have to either:

  1. Restrict possible default values (like in the code above for non-None optional defaults).
  2. Change the order elements are listed in the union schema based on provided default value which is also not ideal.

@bplommer
Copy link
Member

This looks great, thanks!

Which kinda works, but because in avro default values for union fields have to correspond to the first schema in the union would have to either:

Restrict possible default values (like in the code above for non-None optional defaults).
Change the order elements are listed in the union schema based on provided default value which is also not ideal.

I think it's best to leave this as is to avoid too much complexity.

@vlovgr any thoughts on this before I merge?

@bplommer bplommer requested a review from vlovgr March 28, 2021 19:22
@vlovgr
Copy link
Contributor

vlovgr commented Mar 29, 2021

Agreed, let's keep the derivation module as simple as possible.

@bplommer bplommer merged commit 008efcb into fd4s:master Mar 29, 2021
@gafiatulin gafiatulin deleted the default-value-nullable-fields branch March 29, 2021 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants