DGS-8783 Add rulesToMerge/rulesToRemove to tags API #2789
Conversation
| Map<String, String> headerProperties = requestHeaderBuilder.buildRequestHeaders( | ||
| headers, schemaRegistry.config().whitelistHeaders()); | ||
| RegisterSchemaResponse registerSchemaResponse; | ||
| try { | ||
| RuleSet ruleSet = request.getRuleSet(); | ||
| if (request.getRulesToMerge() != null || request.getRulesToRemove() != null) { | ||
| int oldVersion = request.getNewVersion() != null ? request.getNewVersion() - 1 : -1; |
There was a problem hiding this comment.
Does -1 mean latest version?
| } | ||
|
|
||
| @JsonProperty("rulesToMerge") | ||
| public void setRulesToMerge(RuleSet rulesToMerge) { |
There was a problem hiding this comment.
Why not take List as a parameter? Or should we call it setRuleSetToMerge?
There was a problem hiding this comment.
rulesToMerge takes a RuleSet, and rulesToRemove takes a list of rule names. By "rules" I'm referring to the object semantically, not to the data structure that is passed in. I'd rather not call it ruleSetToMerge and ruleNamesToRemove
There was a problem hiding this comment.
IMO I think rulesToRemove is better than ruleNamesToRemove. But I think ruleSetToMerge might better than rulesToMerge. However, it seems like the field that the customer sets is rulesToMerge. What do you think about changing that json field to ruleSetToMerge since it has the same attributes as the existing ruleSet field?
There was a problem hiding this comment.
I'd rather keep it rulesToMerge. In any case, this API may be temporary.
core/src/test/java/io/confluent/kafka/schemaregistry/rest/RestApiRegisterSchemaTagsTest.java
Outdated
Show resolved
Hide resolved
| // Use the previous ruleSet instead of the passed in one | ||
| ruleSet = oldSchema != null ? oldSchema.getRuleSet() : null; | ||
| if (request.getRulesToMerge() != null) { | ||
| ruleSet = mergeRuleSets(ruleSet, request.getRulesToMerge()); |
There was a problem hiding this comment.
Does this change the ordering of the rules? If so does that matter? I remember you mentioning that the order of the ruse matter.
It seems like merging the rules would remove any existing role that has the same name as any rule in rulesToMerge, then add rulesToMerge at the end of the rules list.
There was a problem hiding this comment.
Yes, this code will prefer the order of the rulesToAdd ruleSet over the existing ruleSet. I think this is fine for encryption rules, which is what this API is meant to be used for currently.
.../src/main/java/io/confluent/kafka/schemaregistry/rest/resources/SubjectVersionsResource.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/confluent/kafka/schemaregistry/rest/resources/SubjectVersionsResource.java
Show resolved
Hide resolved
Add rulesToMerge/rulesToRemove to tags API