NewsResourceType is not really an enum and incorrectly ignores valid types

[SimonMarquis]

Context

The current news resource type is defined like this:

nowinandroid/core/model/src/main/java/com/google/samples/apps/nowinandroid/core/model/data/NewsResourceType.kt

Lines 22 to 27 in 153b34f

	enum class NewsResourceType(
	val serializedName: String,
	val displayText: String,
	// TODO: descriptions should probably be string resources
	val description: String,
	) {

And uses this manual deserializing method instead of regular KotlinX Serialization:

nowinandroid/core/model/src/main/java/com/google/samples/apps/nowinandroid/core/model/data/NewsResourceType.kt

Lines 75 to 80 in 153b34f

	fun String?.asNewsResourceType() = when (this) {
	null -> NewsResourceType.Unknown
	else -> NewsResourceType.values()
	.firstOrNull { type -> type.serializedName == this }
	?: NewsResourceType.Unknown
	}

Issues

1. It seems like val description: String was never used
2. val serializedName: String and val displayText: String are always identical
3. The value used from the network payload is not really an enum, but rather a "display" value (contrary to what is documented in How is that Push-based synchronization implemented in NIA? provide a better guide for beginners #886 (comment)).
4. When unknown values are received, the Unknown enum is used instead.
   • This means an "unknown" value will be persisted to the database NewsResourceTypeConverter and will never be decoded correctly, even after an update where the appropriate enum would be backported.
   • This also means, we are currently ignoring many entries: DAC - Android version features and Jetpack release 🚀 from the production environment.

Solution

What I would propose is to remove this enum data type, as it is only used as a display value.
If we later want to map these values as enums to show descriptions, we can definitely add this, but as an external mapper and not baked into the database.

WDYT?

• Replace NewsResourceType enum with simpler String type #902

[tunjid]

Thanks for the suggestion Simon!

When initially designed it was intended to be an enum, though as you pointed out as the app evolved the enum is no longer strict.

We'll discuss internally if we want to coerce the violating types into the established enums, or loosen the type as you've done.

Your assertion:

An "unknown" value will be persisted to the database NewsResourceTypeConverter and will never be decoded correctly, even after > an update where the appropriate enum would be backported.

Is also correct as the changelist will not be updated after an app update, and so the app will only ever see "Unknown".

[dturner]

Yes, great point. In the original spec we had the type as an enum so that the news resources could easily be filtered by type:

• by the user (e.g. show me only articles)
• for other platforms where it made sense (e.g. show only videos on Android TV)
• in more advanced search (e.g. find videos and articles containing "modularization")

These features haven't been implemented, and given that it's relatively trivial to add this back in, it makes sense to change to the simpler String type.

This also means, we are currently ignoring many entries: DAC - Android version features and Jetpack release 🚀 from the production environment.

Thankfully there aren't any news resources with these types. The type strings currently in use are:

Article 📚
Video 📺
Podcast 🎙
Codelab
Docs 📑

Note the addition of emojis in all of these except "Codelab". We should use the emojis in the test data as well. I'll add a comment in the PR.

[SimonMarquis]

In the original spec we had the type as an enum so that the news resources could easily be filtered by type

I understand the initial idea of enums. Although, I think that it could also be done fairly easily with a SELECT DISTINCT type SQL request on all the news entities.
Having an enum type somewhere in an API is always a "risk". Especially if it's not enforced on both client and server sides. And dealing with breaking updates (adding a new entry) have its own challenges 😅

Thankfully there aren't any news resources with these types.

That is not true unfortunately (at the moment) 😬

curl -s 'https://now-in-android-production.appspot.com/newsresources' | 
  jq '.data | group_by(.type) | map({key: .[0].type, value: map(.type) | length}) | sort_by(.key)  | from_entries'

{
  "": 2,
  "Article 📚": 181,
  "Codelab": 9,
  "DAC - Android version features": 10,
  "Docs 📑": 16,
  "Event 📆": 3,
  "Jetpack release 🚀": 25,
  "Podcast 🎙": 21,
  "Video 📺": 138
}

Note the addition of emojis in all of these except "Codelab". We should use the emojis in the test data as well.

I'll restore them 👍

[dturner]

That is not true unfortunately (at the moment) 😬

Oh wow, you're right! I pulled my data from a local spreadsheet which was filtering out some rows. Your query is the correct way to verify this, thanks for sharing.

I'll restore them 👍

Thanks for doing this.

We just need to fix that failing test.

[SimonMarquis]

To fix the testMetaDataDisplay_withUnknownResource test, I think it would be better to assert on a semantic tag instead of trying to find the date String. WDYT?

Nevermind, I found the issue.

[SimonMarquis]

Resolved in #902