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

FIX: reverted deprecated fields on testSuite #19284

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

sushi30
Copy link
Contributor

@sushi30 sushi30 commented Jan 8, 2025

Problem

Following #19221, migrations fail because MigrationUtils rely on objects which use the old schema that contains deprecated fields.

Example Failure

Failed to db migration due to 
org.openmetadata.service.exception.UnhandledServerException: An exception with message [Failed to process JSON ] was thrown while processing request.
	at org.openmetadata.service.util.JsonUtils.readValue(JsonUtils.java:186)
	at org.openmetadata.service.migration.utils.v157.MigrationUtil.migrateTableDiffParams(MigrationUtil.java:36)
	at org.openmetadata.service.migration.postgres.v157.Migration.runDataMigration(Migration.java:18)
...
aused by: com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized field "executable" (class org.openmetadata.schema.tests.TestSuite), not marked as ignorable (34 known properties: "domain", "testCaseResultSummary", "votes", "lifeCycle", "name", "testConnectionResult", "updatedAt", "serviceType", "pipelines", "followers", "version", "deleted", "basicEntityReference", "id", "fullyQualifiedName", "connection", "owners", "tests", "summary", "href", "changeDescription", "displayName", "basic", "style", "updatedBy", "certification", "description", "extension", "experts", "reviewers", "tags", "inherited", "dataProducts", "children"])
 at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 650] (through reference chain: org.openmetadata.schema.tests.TestCase["testSuites"]->java.util.ArrayList[0]->org.openmetadata.schema.tests.TestSuite["executable"])

Proposed Solution

Keep the deprecated along with a useful description regarding the field that replaces it as well as adding the @Deprecated annotation for the field in the created java object. The only context where these deprecated fields should be used is in migrations prior to the version where it was deprecated.

Example deprecated field

/**
 * DEPRECATED in 1.6.2: Use 'basic'
 * 
 */
@JsonProperty("executable")
@Deprecated
public Boolean getExecutable() {
    return executable;
}

Future work

These fields will might be included in some contexts where the json documents are consumed fully (API responses. elasticsearch, etc). Need to make sure they are cleaned up by default in these places.

Describe your changes:

Fixes

Type of change:

  • Bug fix

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

Added deprecated fields with a notice. This is to ensure migrations that use them do not break.
Copy link
Contributor

github-actions bot commented Jan 8, 2025

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 64%
64.59% (40790/63157) 40.81% (16365/40102) 43.97% (4951/11260)

Copy link

sonarqubecloud bot commented Jan 8, 2025

@sushi30 sushi30 enabled auto-merge (squash) January 8, 2025 15:56
Copy link

sonarqubecloud bot commented Jan 8, 2025

@harshach harshach disabled auto-merge January 9, 2025 02:34
@harshach harshach merged commit 7d05902 into main Jan 9, 2025
39 of 50 checks passed
@harshach harshach deleted the fix-test-suite-deprecated-field branch January 9, 2025 02:34
Copy link
Contributor

github-actions bot commented Jan 9, 2025

Changes have been cherry-picked to the 1.6.2 branch.

github-actions bot pushed a commit that referenced this pull request Jan 9, 2025
Added deprecated fields with a notice. This is to ensure migrations that use them do not break.

(cherry picked from commit 7d05902)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ingestion safe to test Add this label to run secure Github workflows on PRs To release Will cherry-pick this PR into the release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants