Validate the subscription topic on client #1197
Conversation
Also renamed `Signal.typeUrl()` -> `Signal.enclosedTypeUrl()` to avoid clashes with `KnownMessage.typeUrl()`.
|
@armiol, @dmdashenkov PTAL. The build depends on SpineEventEngine/base#498. |
| public interface FilterMixin { | ||
|
|
||
| @SuppressWarnings("override") // Implemented in the generated code. | ||
| FieldPath getFieldPath(); |
There was a problem hiding this comment.
Instead of this, you could extend the interface from FilterOrBuilder.
| /** | ||
| * Checks if the target field is a top-level field. | ||
| */ | ||
| default boolean fieldIsTopLevel() { |
There was a problem hiding this comment.
Maybe, fieldAtTopLevel?
Same below.
| * @throws IllegalArgumentException | ||
| * if the target field is not present in the type or doesn't satisfy the constraints | ||
| */ | ||
| default void validateAgainst(TypeUrl targetType) { |
There was a problem hiding this comment.
This is way too broad. It looks closer to checkCanFilter(TypeUrl targetType). It would be even better if checkCanApplyTo(Target target), but I am not sure that's possible.
| * Extends the {@link Target} with validation routines. | ||
| */ | ||
| @GeneratedMixin | ||
| @SuppressWarnings("override") // Methods are implemented in the generated code. |
There was a problem hiding this comment.
See my comment above on this matter.
...to avoid manually declaring the proto getters.
Codecov Report
@@ Coverage Diff @@
## master #1197 +/- ##
===========================================
+ Coverage 91.4% 91.4% +<.01%
- Complexity 4214 4231 +17
===========================================
Files 545 547 +2
Lines 13129 13189 +60
Branches 744 749 +5
===========================================
+ Hits 12000 12055 +55
- Misses 894 896 +2
- Partials 235 238 +3 |
|
@armiol PTAL again. |
| option java_outer_classname = "EventsProto"; | ||
| option java_multiple_files = true; | ||
|
|
||
| message ClCreateProject { |
There was a problem hiding this comment.
It's misleading that a file called events.proto has one command and one value type in it.
| * Extends the {@link Target} with validation routines. | ||
| */ | ||
| @GeneratedMixin | ||
| public interface TargetMixin extends TargetOrBuilder { |
There was a problem hiding this comment.
Please make the mixin interfaces package-private. If they are public by design, please document the reasons, as usually that is not the case.
There was a problem hiding this comment.
@dmitrykuzmin LGTM except for the comments by @dmdashenkov.
…into validate-client-subscriptions � Conflicts: � license-report.md
This PR addresses #1191.
It introduces the
TargetMixinwhich allows to check if the target is valid.By "valid" we assume the target is of
EntityState/EventMessagetype and has filters which target existing message fields.By default, the check is run in
TopicBuilderon topic creation. Thus, all "standard" API for topic creation will run the check. But, if the topic is manually created and passed to theSubscriptionService, it should be checked also manually.For queries, the target validness check is not run, as the mistake in declared type/filters will be revealed at once, removing the need to double check it on the client.
Spine version advances to
1.2.3.