[Issue 9602] Add schema type validation #9797
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/pulsarbot run-failure-checks |
|
@congbobo184 @gaoran10 Please help review this PR. |
codelipenghui
added
component/schemaregistry
type/bug
codelipenghui
changed the title
codelipenghui
requested review from
315157973,
hangc0276,
jiazhai,
merlimat,
sijie and
zymap
|
|
||
| import com.google.common.collect.Sets; | ||
| import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest; | ||
| import org.apache.pulsar.client.api.*; |
There was a problem hiding this comment.
It's better to avoid star import.
| import java.util.concurrent.ExecutionException; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| import static org.testng.Assert.*; |
There was a problem hiding this comment.
Same as the above comment.
| private final String namespace = "test-namespace"; | ||
| private final String namespaceName = PUBLIC_TENANT + "/" + namespace; | ||
|
|
||
| @BeforeMethod |
There was a problem hiding this comment.
Is it possible to change to BeforeClass? This can avoid start and shutdown the mock service for each test.
codelipenghui
approved these changes
Mar 5, 2021
sijie
requested changes
Mar 8, 2021
| @@ -140,6 +140,15 @@ | |||
| SchemaCompatibilityStrategy strategy) { | |||
| return trimDeletedSchemaAndGetList(schemaId).thenCompose(schemaAndMetadataList -> | |||
| getSchemaVersionBySchemaData(schemaAndMetadataList, schema).thenCompose(schemaVersion -> { | |||
| if (schemaAndMetadataList.size() > 0) { | |||
| for (SchemaAndMetadata metadata : schemaAndMetadataList) { | |||
There was a problem hiding this comment.
Don't you need to check if strategy is not ALWAYS_COMPATIBLE?
There was a problem hiding this comment.
There is really a need to check the strategy type, so I will add that and unit tests.
| compatibilityChecks.getOrDefault(newSchema.getType(), SchemaCompatibilityCheck.DEFAULT) | ||
| .checkCompatible(existingSchemaData, newSchema, strategy); | ||
| } | ||
| if (newSchema.getType() != existingSchemaData.getType()) { |
There was a problem hiding this comment.
Don't you need to check if strategy is not ALWAYS_COMPATIBLE?
There was a problem hiding this comment.
There is no need to check compatibility here. Because in checkConsumerCompatibility will judge. If it is ALWAYS_COMPATIBLE, it will not be called here.
|
@sijie Please review this PR again, thanks. |
eolivelli
approved these changes
Mar 10, 2021
jiazhai
approved these changes
Mar 12, 2021
sijie
approved these changes
Mar 12, 2021
fmiguelez
pushed a commit
to fmiguelez/pulsar
that referenced
this pull request
Mar 16, 2021
Fixes apache#9602 ### Motivation The schema should not be able to modify the type after it is first created. Unless he is Always compatible. ### Modifications When uploading a schema, it is verified that the type of the newly uploaded schema is consistent with the schema in the metadata. When checkCompatible, change the Primitive type from checking only to checking all. ### Verifying this change * When a producer is created, the schema is defined, and when a producer is created again, the schema type is not always the same, otherwise an exception will be thrown. * When a producer is created, the schema is defined, and when a consumer is created again, the schema type is not always the same, otherwise an exception will be thrown. * When a consumer is created, the schema is defined, and when a producer is created again, the schema type is not always the same, otherwise an exception will be thrown. * When a consumer is created, the schema is defined, and when a consumer is created again, the schema type is not always the same, otherwise an exception will be thrown. * When compatibility is set to always compatible. No exceptions are thrown. ### Does this pull request potentially affect one of the following parts: *If `yes` was chosen, please highlight the changes* - The schema: (yes)
codelipenghui
pushed a commit
that referenced
this pull request
Mar 23, 2021
Fixes #9602 The schema should not be able to modify the type after it is first created. Unless he is Always compatible. When uploading a schema, it is verified that the type of the newly uploaded schema is consistent with the schema in the metadata. When checkCompatible, change the Primitive type from checking only to checking all. * When a producer is created, the schema is defined, and when a producer is created again, the schema type is not always the same, otherwise an exception will be thrown. * When a producer is created, the schema is defined, and when a consumer is created again, the schema type is not always the same, otherwise an exception will be thrown. * When a consumer is created, the schema is defined, and when a producer is created again, the schema type is not always the same, otherwise an exception will be thrown. * When a consumer is created, the schema is defined, and when a consumer is created again, the schema type is not always the same, otherwise an exception will be thrown. * When compatibility is set to always compatible. No exceptions are thrown. *If `yes` was chosen, please highlight the changes* - The schema: (yes) (cherry picked from commit b45adde)
codelipenghui
added a commit
that referenced
this pull request
Apr 26, 2021
Related to #9797 ### Motivation Fix schema type check issue when use always compatible strategy. 1. For non-transitive strategy, only check schema type for the last schema 2. For transitive strategy, check all schema's type 3. Get schema by schema data should consider different schema types
codelipenghui
added a commit
that referenced
this pull request
Apr 27, 2021
Related to #9797 Fix schema type check issue when use always compatible strategy. 1. For non-transitive strategy, only check schema type for the last schema 2. For transitive strategy, check all schema's type 3. Get schema by schema data should consider different schema types (cherry picked from commit 04f8c96)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #9602
Motivation
The schema should not be able to modify the type after it is first created. Unless he is Always compatible.
Modifications
When uploading a schema, it is verified that the type of the newly uploaded schema is consistent with the schema in the metadata.
When checkCompatible, change the Primitive type from checking only to checking all.
Verifying this change
When a producer is created, the schema is defined, and when a producer is created again, the schema type is not always the same, otherwise an exception will be thrown.
When a producer is created, the schema is defined, and when a consumer is created again, the schema type is not always the same, otherwise an exception will be thrown.
When a consumer is created, the schema is defined, and when a producer is created again, the schema type is not always the same, otherwise an exception will be thrown.
When a consumer is created, the schema is defined, and when a consumer is created again, the schema type is not always the same, otherwise an exception will be thrown.
When compatibility is set to always compatible. No exceptions are thrown.
Does this pull request potentially affect one of the following parts:
If
yeswas chosen, please highlight the changes