Bugzilla – Full Text Bug Listing
| Summary: | Handlers registered w/ topic a/b/c/* should not receive events on topic a/b/c | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | Alan D. Cabrera <adc> | ||||||||
| Component: | Compendium | Assignee: | John Ross <jwross> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | hargrave, john.arthorne, jwross, ob1.eclipse, tjwatson | ||||||||
| Version: | unspecified | Flags: | tjwatson:
review+
|
||||||||
| Target Milestone: | 3.7 M4 | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Alan D. Cabrera
Assigning to John. (In reply to comment #0) > In Equinox avent admin, event.topics a/b/c/* matches a/b/c. They should not. My initial reaction was to agree. However, while reviewing the spec, I came across this. "A wildcard (’*’ \u002A) may be used as the last token of a topic name, for example com/action/*. This matches any topic that shares the same first tokens. For example, com/action/* matches com/action/listen." Although the example in the spec provides no support, matching "a/b/c" against "a/b/c/*" seems to fall under the definition of matching "any topic that shares the same first tokens". Nothing is mentioned about the significance of the intervening level. Unless I can find more information in the spec, it seems this might be open to interpretation. Alan, were you basing this on something you saw in the spec? (In reply to comment #2) > (In reply to comment #0) > > In Equinox avent admin, event.topics a/b/c/* matches a/b/c. They should not. > > My initial reaction was to agree. However, while reviewing the spec, I came > across this. > > "A wildcard (’*’ \u002A) may be used as the last token of a topic name, for > example com/action/*. This matches any topic that shares the same first tokens. > For example, com/action/* matches com/action/listen." > > Although the example in the spec provides no support, matching "a/b/c" against > "a/b/c/*" seems to fall under the definition of matching "any topic that shares > the same first tokens". Nothing is mentioned about the significance of the > intervening level. > > Unless I can find more information in the spec, it seems this might be open to > interpretation. > > Alan, were you basing this on something you saw in the spec? See https://mail.osgi.org/pipermail/osgi-dev/2010-September/002629.html I think the confusion is around what the term token mean in the specification here. Is '/' considered a token or not? If '/' is a token then a/b/c/* should not match a/b/c / is not a token. It is a separator between tokens.
com/action/* matches com/action/listen because com and action are tokens and must match exactly and * matches any third token. In java, the can be thought of as "com/action/listen".startsWith("com/action/"). Note the trailing * is remove and the / separator remains. Thus "com/action" does not match.
(In reply to comment #4) > / is not a token. It is a separator between tokens. > com/action/* matches com/action/listen because com and action are tokens and > must match exactly and * matches any third token. In java, the can be thought > of as "com/action/listen".startsWith("com/action/"). Note the trailing * is > remove and the / separator remains. Thus "com/action" does not match. Okay. My only concern was whether or not this change could possibly violate the spec. I think it's the way to go because if I was interested in a/b/c and all sublevels of a/b/c I could register an EVENT_TOPIC of new String[] {"a/b/c", "a/b/c/*"}. With the current functionality, I don't see a way to do this if I was only interested in sublevels of a/b/c but not a/b/c itself. (In reply to comment #4) > / is not a token. It is a separator between tokens. > > com/action/* matches com/action/listen because com and action are tokens and > must match exactly and * matches any third token. In java, the can be thought > of as "com/action/listen".startsWith("com/action/"). Note the trailing * is > remove and the / separator remains. Thus "com/action" does not match. Please see comment 2 where the spec states: "A wildcard (’*’ \u002A) may be used as the last token of a topic name, for example com/action/*. This matches any topic that shares the same first tokens. For example, com/action/* matches com/action/listen." If / is not a token the a/b/c is comprised of 3 tokens (a, b, c) and a/b/c/* is comprised of 4 tokens (a, b, c, *). So to me a/b/c shares the same first three tokens of a/b/c/* Correct? (In reply to comment #6) > Please see comment 2 where the spec states: > > "A wildcard (’*’ \u002A) may be used as the last token of a topic name, for > example com/action/*. This matches any topic that shares the same first tokens. > For example, com/action/* matches com/action/listen." > > If / is not a token the a/b/c is comprised of 3 tokens (a, b, c) and a/b/c/* is > comprised of 4 tokens (a, b, c, *). So to me a/b/c shares the same first three > tokens of a/b/c/* > > Correct? But there is no fourth token to match. So, FAIL. You still need a fourth token to match. In that case of * that will match any fourth token but will not match the absence of a fourth token. John is correct in comment 5. (In reply to comment #7) > But there is no fourth token to match. So, FAIL. You still need a fourth token > to match. In that case of * that will match any fourth token but will not match > the absence of a fourth token. I wonder of the spec really meant to say: "A wildcard (’*’ \u002A) may be used as the last token of a topic name, for example com/action/*. This matches any [token] that shares the same first tokens." Using the word "topic" still leaves some room for ambiguity in my mind. But I still agree we should make this change as long as we're sure it doesn't violate the spec and have no backwards compatibility concerns. Hey John A. Do you know if e4 usage of EventAdmin would be affected by this change in behavior? Created attachment 181564 [details]
Proposed Patch
This patch updates EventAdmin to no longer deliver events published to a handler using a wildcard topic when there is no token to match at the wildcard level. For example, an event published on "a/b/c" will no longer be delivered to a handler listening to "a/b/c/*". This turned out to be a simple, one line code change.
I also took the opportunity to set the EventAdmin's project specific settings so that it checks for invalid references to all specified execution environments.
Created attachment 181565 [details]
Test Cases
This patches org.eclipse.equinox.compendium.tests by adding test cases related to this bug.
Patch looks good. I would like to confirm with John A. before releasing. Thinking a bit more, the current behavior does seem unreasonable. Even if e4 somehow uses this behavior we should look to change e4 to be correct. Created attachment 181644 [details]
Updated Test Cases
Adds one additional test case that ensures event delivery on topics "a/b/c" and "a/b/c/d" when listening to an EVENT_TOPIC of new String[]{"a/b/c", "a/b/c/*"}.
This represents the impact to users relying on the current functionality (i.e. an event handler would need to register one additional topic representing the level immediately before the wildcard).
Comment #9 is best answered by Oleg - he wrote all the event support in e4. (In reply to comment #7) > (In reply to comment #6) > > Please see comment 2 where the spec states: > > > > "A wildcard (’*’ \u002A) may be used as the last token of a topic name, for > > example com/action/*. This matches any topic that shares the same first tokens. > > For example, com/action/* matches com/action/listen." > > > > If / is not a token the a/b/c is comprised of 3 tokens (a, b, c) and a/b/c/* is > > comprised of 4 tokens (a, b, c, *). So to me a/b/c shares the same first three > > tokens of a/b/c/* > > > > Correct? > But there is no fourth token to match. So, FAIL. You still need a fourth token > to match. In that case of * that will match any fourth token but will not match > the absence of a fourth token. > John is correct in comment 5. After reading and re-reading and re-re-reading this, I believe that what is *written* in the spec says that "a/b/c/*" matches "a/b/c". The key phrase in the spec is: "This matches any topic that shares the same first tokens." Not "all tokens", but "first tokens". I, of course, can't read minds of the spec authors; they might have had something else in mind :-). So, +1 to Tom's comments. As the spec does not explicitly define this behaviour and only implies it, it could be considered ambiguous and open to interpretation. From the comments here I haven't seen any arguments as to why one behaviour would be preferable to another. As such, I'd be inclined to leave it as it is. From the e4 perspective, the EMF integration layer uses trailing "/*" in topics; see, for instance, UIEvents#buildTopic(). The event system is open-ended so it is hard to say what effect this change will have. (In reply to comment #15) > From the comments here I haven't seen any arguments as to why one behaviour > would be preferable to another. As such, I'd be inclined to leave it as it is. Comment 5 is the kicker for me. With the current functionality, it is impossible for a user to only receive events published to sublevels of a/b/c (e.g., a/b/c/d, a/b/d/e, and a/b/c/f) without also receiving events published to a/b/c, even if they are of no interest or perhaps even harmful. This forces users to filter these events out themselves, negating one of the primary advantages of EventAdmin. The new functionality is unlimiting. Users can still obtain the previous functionality by registering an EVENT_TOPIC of new String[] {"a/b/c", "a/b/c/*"}. (In reply to comment #15) > After reading and re-reading and re-re-reading this, I believe that what is > *written* in the spec says that "a/b/c/*" matches "a/b/c". The key phrase in > the spec is: "This matches any topic that shares the same first tokens." Not > "all tokens", but "first tokens". > This is just simple globbing of the last components of the path. I fail to see why this is so hard for people to read. a/b/c/* requires a topic containing at least 4 parts and the parts after the first 3 can be anything. a/b/c is a 3 part topic and therefore cannot match a 4 or more part topic. Even it you assume the empty string can match *, a/b/c/ is not a legal topic and also does not match a/b/c. > I, of course, can't read minds of the spec authors; they might have had > something else in mind :-). I am the spec author. The intention of the spec is that a/b/c/* does *not* match a/b/c. > As the spec does not explicitly define this behaviour and only implies it, it > could be considered ambiguous and open to interpretation. No. > From the comments here I haven't seen any arguments as to why one behaviour > would be preferable to another. So one can match a/b/c/d but not a/b/c. > As such, I'd be inclined to leave it as it is. Then you are failing to implement the spec and will fail the compliance test. > From the e4 perspective, the EMF integration layer uses trailing "/*" in > topics; see, for instance, UIEvents#buildTopic(). The event system is > open-ended so it is hard to say what effect this change will have. If you want events a/b/c and a/b/c/* then use both filters. I think we better go ahead and get this released. We are not compliant with the specification and need to bring the impl back up to spec. (In reply to comment #15) > (In reply to comment #7) > From the e4 perspective, the EMF integration layer uses trailing "/*" in > topics; see, for instance, UIEvents#buildTopic(). The event system is > open-ended so it is hard to say what effect this change will have. So is the topic building code all in one place (e.g. UIEvent#buildTopic())? If so and e4 is really concerned then they can add two topics when registering the listener, one with a trailing /* and one without. I want to release this patch after tomorrows I-Build (11/16/2010 build). Patch released. |