Skip to content

Conversation

@jonathannorris
Copy link
Member

The JSON parsing library was audited to ensure unknown properties are ignored globally.

  • It was confirmed that JSONMapper.kt is configured with DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES set to false, which globally ignores unknown JSON properties during deserialization.
  • Consequently, all redundant @JsonIgnoreProperties(ignoreUnknown = true) annotations were removed from the following model classes:
    • BucketedUserConfig.kt
    • Variable.kt
    • ConfigVariable.kt (6 instances across abstract and concrete classes)
    • SSE.kt
    • ProjectSettings.kt
    • Project.kt
    • Feature.kt
    • ErrorResponse.kt
    • Environment.kt
    • EdgeDB.kt
    • DevCycleUser.kt
  • Unused import com.fasterxml.jackson.annotation.JsonIgnoreProperties statements were also removed.
  • Other JSON annotations (@JsonProperty, @JsonInclude, @JsonFormat, @JsonDeserialize) were retained as they serve distinct, necessary purposes for serialization, formatting, and custom deserialization logic.

Copy link
Contributor

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 cleans up the models by removing redundant @JsonIgnoreProperties annotations now that the global JSON configuration ignores unknown properties. It also updates several classes to Kotlin data classes for improved readability and consistency, and removes unused imports.

  • Removed redundant JSON annotations from multiple model classes.
  • Converted regular classes to data classes.
  • Removed unused or unnecessary import statements.

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
Variable.kt Removed unused JSON annotation and updated comment formatting.
SSE.kt Removed JSON annotation and converted class to a data class.
ProjectSettings.kt Removed JSON annotation and converted class to a data class.
Project.kt Removed JSON annotation and converted class to a data class.
Feature.kt Removed JSON annotation and converted class to a data class.
ErrorResponse.kt Removed JSON annotation and replaced with a wildcard import.
Environment.kt Removed JSON annotation, converted class to a data class, and reformatted code.
EdgeDB.kt Removed JSON annotation and converted class to a data class.
DevCycleUser.kt Removed JSON annotation from the Builder and converted class to a data class.
ConfigVariable.kt Removed JSON annotations from multiple variable classes.
BucketedUserConfig.kt Removed redundant JSON annotation.

* Feature
*/
class Feature {
data class Feature {
Copy link
Member Author

Choose a reason for hiding this comment

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

this is interesting that this was missing...

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess do we want these to be data classes?

Copy link
Contributor

@kaushalkapasi kaushalkapasi Jun 19, 2025

Choose a reason for hiding this comment

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

yeah, I just made the same update in my PR #240. though it does need some refactoring to make sure everything compiles, the test data is updated and all test pass appropriately

@jonathannorris jonathannorris changed the title Cleanup unnecessary JSON properties and annotations chore: cleanup unnecessary JSON properties and annotations Jun 20, 2025
@jonathannorris jonathannorris requested a review from suthar26 June 20, 2025 15:44
@jonathannorris jonathannorris merged commit fc39179 into main Jun 20, 2025
5 of 6 checks passed
@jonathannorris jonathannorris deleted the cursor/cleanup-unnecessary-json-properties-and-annotations-f22b branch June 20, 2025 18:12
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.

6 participants