Skip to content

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Jul 15, 2025

This pull request introduces components for supporting user-defined enums in the firebase-dataconnect library. It defines the EnumValue type capable of representing both recognized and unrecognized enum values, along with a dedicated serializer for kotlinx.serialization. These types duplicate those generated by the Data Connect code generator; however, in the future the code generator will cease to generate them and, instead, use the types added in this PR.

Highlights

  • New Data Type for Enums: Introduced EnumValue, a sealed interface designed to represent enum values. It has two concrete implementations: EnumValue.Known for recognized enum members and EnumValue.Unknown for string values that do not map to a known enum member. This design allows for graceful handling of forward compatibility, such as when new enum values are added to an API.
  • Serialization Support: Added EnumValueSerializer, a kotlinx.serialization.KSerializer implementation for EnumValue. This serializer handles converting enum strings to EnumValue objects (mapping to Known or Unknown as appropriate) and serializing EnumValue objects back to their string representation.

Google employees can see go/dataconnect:sdk:enums for full details.

This comment was marked as off-topic.

gemini-code-assist[bot]

This comment was marked as outdated.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces EnumValue and EnumValueSerializer to handle enums that may have values not known at compile time. The review suggests refactoring the EnumValue sealed class implementations to use Kotlin's data class feature to reduce boilerplate.

@google-oss-bot

This comment was marked as outdated.

@google-oss-bot

This comment was marked as outdated.

@dconeybe dconeybe marked this pull request as ready for review July 15, 2025 20:06
@dconeybe dconeybe requested a review from Copilot July 15, 2025 20:06
Copilot

This comment was marked as outdated.

@dconeybe dconeybe changed the title dataconnect: EnumValue and EnumValueSerializer added, but not public yet dataconnect: EnumValue and EnumValueSerializer added, but internal for now Jul 15, 2025
@dconeybe dconeybe requested a review from aashishpatil-g July 17, 2025 20:16
@dconeybe dconeybe changed the title dataconnect: EnumValue and EnumValueSerializer added, but internal for now dataconnect: EnumValue and EnumValueSerializer added Sep 8, 2025
@dconeybe dconeybe requested a review from Copilot September 9, 2025 00:00
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces enum value support components for the firebase-dataconnect library, adding EnumValue types and serialization infrastructure to handle both known and unknown enum values gracefully.

  • Adds EnumValue sealed interface with Known and Unknown implementations for forward-compatible enum handling
  • Implements EnumValueSerializer for kotlinx.serialization support
  • Updates integration tests to demonstrate enum value serialization with subset mappings

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/EnumValue.kt Core EnumValue sealed interface and implementations
firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/serializers/EnumValueSerializer.kt Kotlinx serialization support for EnumValue
firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/EnumValueKnownUnitTest.kt Unit tests for EnumValue.Known implementation
firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/EnumValueUnknownUnitTest.kt Unit tests for EnumValue.Unknown implementation
firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/serializers/EnumValueSerializerUnitTest.kt Unit tests for EnumValueSerializer
firebase-dataconnect/src/androidTest/kotlin/com/google/firebase/dataconnect/EnumIntegrationTest.kt Integration tests demonstrating enum value usage
firebase-dataconnect/api.txt API surface updates for new public classes
firebase-dataconnect/gradle.properties Version bump to 17.1.0
firebase-dataconnect/CHANGELOG.md Documentation of new features

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@dconeybe
Copy link
Contributor Author

dconeybe commented Sep 9, 2025

/gemini review

Copy link
Contributor

Summary of Changes

This pull request integrates robust support for user-defined enums within the firebase-dataconnect library. It provides a flexible EnumValue type that can represent both expected and unexpected enum strings, along with a dedicated serializer. This foundational change prepares the library for future updates where these types will be directly consumed, enhancing API stability and developer experience.

Highlights

  • New Enum Value Type: Introduces EnumValue, a sealed interface with Known and Unknown implementations, to gracefully handle recognized and unrecognized enum values, ensuring forward compatibility.
  • Kotlinx Serialization Support: Adds EnumValueSerializer for kotlinx.serialization, enabling proper serialization and deserialization of EnumValue objects to and from string representations.
Changelog
  • firebase-dataconnect/CHANGELOG.md
    • Updated with an entry for EnumValue and EnumValueSerializer, indicating their role in replacing code-generated types.
  • firebase-dataconnect/api.txt
    • API surface updated to include the new EnumValue sealed interface (with Known and Unknown nested classes) and EnumValueSerializer.
  • firebase-dataconnect/gradle.properties
    • Project version incremented to 17.1.0.
  • firebase-dataconnect/src/androidTest/kotlin/com/google/firebase/dataconnect/EnumIntegrationTest.kt
    • Integration tests were updated to utilize the new EnumValue types and EnumValueSerializer for deserialization, including new helper functions for enum conversion.
  • firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/EnumValue.kt
    • New file defining the EnumValue sealed interface, its Known (for valid enum members) and Unknown (for unrecognized strings) implementations, and their respective properties and methods.
  • firebase-dataconnect/src/main/kotlin/com/google/firebase/dataconnect/serializers/EnumValueSerializer.kt
    • New file defining the KSerializer implementation for EnumValue, handling string-to-enum and enum-to-string conversions.
  • firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/EnumValueKnownUnitTest.kt
    • Added comprehensive unit tests for the EnumValue.Known class, covering constructor, properties, equality, hash code, string representation, and copy functionality.
  • firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/EnumValueUnknownUnitTest.kt
    • Added comprehensive unit tests for the EnumValue.Unknown class, covering constructor, properties, equality, hash code, string representation, and copy functionality.
  • firebase-dataconnect/src/test/kotlin/com/google/firebase/dataconnect/serializers/EnumValueSerializerUnitTest.kt
    • Added unit tests for EnumValueSerializer, verifying correct serialization and deserialization of both known and unknown enum values.
Activity
  • Initial automated comments from github-actions[bot] regarding merging policy.
  • Automated reports from google-oss-bot showing a slight decrease in overall coverage (-0.14%) and a minor increase in aar size (+5.60 kB).
  • Author dconeybe initiated summary and review generation via /gemini commands.
  • gemini-code-assist[bot] suggested using Kotlin data class for EnumValue.Known and EnumValue.Unknown to reduce boilerplate; these suggestions were declined by dconeybe due to Firebase's policy against data class in public APIs to avoid breaking changes.
  • Copilot identified and dconeybe resolved typos in TODO comments (visilibity to visibility).
  • Copilot pointed out inconsistent rs parameter usage in tests, which dconeybe addressed.
  • Copilot provided a nitpick suggestion for multi-line type annotation formatting, which was noted but not explicitly resolved in the provided comments.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces EnumValue and EnumValueSerializer to support user-defined enums with forward compatibility in firebase-dataconnect. The implementation is robust and well-tested with both unit and integration tests. My review includes a couple of suggestions to enhance code clarity and conciseness, primarily by leveraging Kotlin's data class feature and improving the structure of test data classes for better maintainability.

@dconeybe dconeybe merged commit 2bed754 into main Sep 16, 2025
46 of 47 checks passed
@dconeybe dconeybe deleted the dconeybe/dataconnect/UserDefinedEnumsInternal branch September 16, 2025 18:46
rlazo added a commit that referenced this pull request Oct 2, 2025
The change #7153 was not part of the 17.0.1 release, so it should be
part of the unreleased section. Also, since it introduces API changes,
it requires a minor version bump.
rlazo added a commit that referenced this pull request Oct 2, 2025
The change #7153 was not part of the 17.0.1 release, so it should be
part of the unreleased section. Also, since it introduces API changes,
it requires a minor version bump.
rlazo added a commit that referenced this pull request Oct 2, 2025
The change #7153 was not part of the 17.0.1 release, so it should be
part of the unreleased section. Also, since it introduces API changes,
it requires a minor version bump.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants