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

Alternative Enum adapter with default value #1812

Open
nikclayton opened this issue Feb 21, 2024 · 8 comments
Open

Alternative Enum adapter with default value #1812

nikclayton opened this issue Feb 21, 2024 · 8 comments

Comments

@nikclayton
Copy link

nikclayton commented Feb 21, 2024

There's already support for parsing enum values with unrecognised values, EnumJsonAdapter with a fallback.

Moshi moshi = new Moshi.Builder()
    .add(CurrencyCode.class, EnumJsonAdapter.create(CurrencyCode.class)
        .withUnknownFallback(CurrencyCode.USD))
    .build();

However, I'm a bit concerned that the choice of default is happening as the adapter is added to Moshi. This can be quite far in the code from where the enum is defined, so whoever is reading the code has to look in multiple places to understand how the enum will be processed.

Below is an alternative I'm offering up for discussion / feedback (I'm sure the code can be improved).

You add the adapter as normal:

val moshi = Moshi.Builder().add(HasDefault.Factory()).build()

and then add two annotations to enums that have defaults; the first marks it as an enum that has a default, and the second marks the actual enum to use as the default:

@HasDefault
enum class CurrencyCode { EUR, CHF, GBP,  @Default USD }

I have a variant that would let you write:

@DefaultEnum(value = "USD")
enum class CurrencyCode { EUR, CHF, GBP, USD }

but I'm not sure of the wisdom of that approach when tools like ProGuard might (without careful configuration) rewrite enum names (this would be much easier to write if annotations could be generic...)

The code's at https://gist.github.com/nikclayton/999558150bfdd48e93f12b8754694eed. If this is interesting to people I'm happy to prep a PR for inclusion.

@nikclayton
Copy link
Author

In particular, this would be very useful to hook up in the generated code to add some companion methods.

E.g., this:

@HasDefault
enum class CurrencyCode { EUR, CHF, GBP,  @Default USD }

should act as though I'd also written something like:

@HasDefault
enum class CurrencyCode { EUR, CHF, GBP,  @Default USD;
    companion object {
        fun default() = CurrencyCode.USD
        fun valueOfOrDefault(s: String) = try { 
                CurrencyCode.valueOf(s)
            } catch (e: IllegalArgumentException) {
                default()
            }
    }
}

@JakeWharton
Copy link
Member

Since the annotation would be new API, we don't really need to require a special adapter be installed. It could be supported by the default enum adapter.

I think I would probably call it @DefaultOnUnknown though.

One reason why this cannot completely obviate the EnumJsonAdapter is that it requires that you control compilation of the enum. While common, this is not guaranteed. Now we have a precedence problem. What happens if you specify both things? And is that behavior intuitive enough?

I'll see what some others think about it.

@nikclayton
Copy link
Author

What happens if you specify both things?

The options are probably:

  1. Compile time error with no workaround
  2. @DefaultOnUnknown is preferred
  3. EnumJsonAdapter is preferred

A compile time error is obviously bad, I think that option can be ignored.

Of the remaining two, I think EnumJsonAdapter should be preferred, on the assumption that an option passed to the Builder is a more explicit signal of user intent than an annotation on an enum (which, as you note, the user compiling the code might not control).

But EnumJsonAdapter should also emit a warning if it's used on an enum that has the @DefaultOnUnknown annotation to inform the user that they're overriding an existing default. That warning should itself be suppressable for users that know what they're doing.

[ Not for the first time I find myself wishing that Kotlin had something like Rust's std::default::Default trait ]

@JakeWharton
Copy link
Member

1 is actually not possible since we aren't guaranteed the use of a compile-time tool, and even if we are we can't know what adapters are added to the Moshi instance.

I agree that the explicit installed adapter is the logical choice to take precedence (in the same way you can override any other built in behavior with a custom adapter).

I'm not sure I'd warn, though (we don't have anywhere to put the warning anyway because this happens entirely at runtime).

@JakeWharton
Copy link
Member

I think I'm convincing myself that doing this in the built-in adapter isn't a good choice. Because it means that the author of the enum has the ability to change the behavior of some downstream module (or even project).

We can, however, still accomplish this with a built-in adapter. Let me take a swing at one real quick.

@nikclayton
Copy link
Author

I think I'm convincing myself that doing this in the built-in adapter isn't a good choice. Because it means that the author of the enum has the ability to change the behavior of some downstream module (or even project).

Isn't that already the case with existing annotations? If the author of the enum has added an @Json(name = ...) annotation at one version, and then changes it some later version of the code that will affect the behaviour of downstream modules or projects.

@JakeWharton
Copy link
Member

Yeah I take your point, but this is a bit different as it deals in fallback behavior rather than simply mapping the Kotlin to JSON.

I feel like if you are changing the fallback behavior for unknown values for the whole JSON tree it should be a bit more of an opt-in thing.

@JakeWharton
Copy link
Member

I'm realizing that you can build this with a helper that delegates to EnumJsonAdapter.

public annotation class DefaultOnUnknown

public inline fun <reified T : Enum<T>> blah(): JsonAdapter<T> {
  return blah(T::class.java)
}

public fun <T : Enum<T>> blah(enumType: Class<T>): JsonAdapter<T> {
  var default: T? = null
  for (enumConstant in enumType.enumConstants) {
    if (enumType.getField(enumConstant.name).isAnnotationPresent(DefaultOnUnknown::class.java)) {
      check(default == null) {
        "Multiple entries annotated with @${DefaultOnUnknown::class.simpleName} on ${enumType.name}"
      }
      default = enumConstant
    }
  }
  requireNotNull(default) {
    "No entry annotated with @${DefaultOnUnknown::class.simpleName} on ${enumType.name}"
  }
  return EnumJsonAdapter.create(enumType).withUnknownFallback(default)
}

However, if you want a full adapter, I also wrote that (mostly by modifying EnumJsonAdapter quickly):

public annotation class DefaultOnUnknown

public class EnumDefaultOnUnknownJsonAdapter<T : Enum<T>>(
  private val enumType: Class<T>,
) : JsonAdapter<T>() {
  private val constants = enumType.enumConstants
  private val default: T
  private val options: Options
  private val nameStrings: Array<String>

  init {
    var default: T? = null
    try {
      nameStrings = Array(constants.size) { i ->
        val constantName = constants[i].name
        val field = enumType.getField(constantName)
        if (field.isAnnotationPresent(DefaultOnUnknown::class.java)) {
          check(default == null) {
            "Multiple entries annotated with @${DefaultOnUnknown::class.simpleName} on ${enumType.name}"
          }
          default = constants[i]
        }
        field.jsonName(constantName)
      }
    } catch (e: NoSuchFieldException) {
      throw AssertionError("Missing field in ${enumType.name}", e)
    }
    this.default = requireNotNull(default) {
      "No entry annotated with @${DefaultOnUnknown::class.simpleName} on ${enumType.name}"
    }
    options = Options.of(*nameStrings)
  }

  @Throws(IOException::class)
  override fun fromJson(reader: JsonReader): T? {
    val index = reader.selectString(options)
    if (index != -1) return constants[index]
    if (reader.peek() != STRING) {
      throw JsonDataException(
        "Expected a string but was ${reader.peek()} at path ${reader.path}",
      )
    }
    reader.skipValue()
    return default
  }

  @Throws(IOException::class)
  override fun toJson(writer: JsonWriter, value: T?) {
    if (value == null) {
      throw NullPointerException(
        "value was null! Wrap in .nullSafe() to write nullable values.",
      )
    }
    writer.value(nameStrings[value.ordinal])
  }

  override fun toString(): String = "EnumDefaultOnUnknownJsonAdapter(${enumType.name})"
}

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

2 participants