[Standards] XEP-0433 Extended Channel Search feedback

Hello! Here's my feedback on XEP-0433 Extended Channel Search: https://xmpp.org/extensions/xep-0433.html The feedback herein is based on two efforts: - Adding support for XEP-0433 to Openfire (which is now reaching a ready-ish state: https://github.com/igniterealtime/Openfire/pull/2756 ) - Adding interop tests to the Xmpp Interop Testing Framework ( https://github.com/XMPP-Interop-Testing/smack-sint-server-extensions/pull/77 ) Particularly the exercise of trying to write tests generated quite a bit of feedback. Without having explicit MUST conditions, it is not possible to write many assertions. This is why a lot of the feedback below suggests making things explicit. 1. I'd like to see a MUST in 4.2 / 4.2.1 that describes that the service to be searched MUST support the retrieval of a search form. That removes ambiguity, I think. I prefer the clarity of the wording in the comparable section 2.1 of XEP-0055: https://xmpp.org/extensions/xep-0055.html#usecases-search That section describes basically the same functionality requesting a form), but it's more explicit. 2. Explicitly define the form type (which is now mostly in an example. It's slightly different from Muclumbus, which can add to the confusion (maybe a 'differences with precursor' section would be handy). 3. The XEP defines two sort keys. It would be good to explicitly define if they are mandatory-to-implement? 4. If the sort keys are meant to be extensible, like the form fields, it would be good to make that more explicit (perhaps with references to a registry, like we do for other XEPs). The XEPs author mentioned in a chat that sort keys are meant to be extensible and neither are mandatory-to-implement. They also wrote that clients must discover support for individual sort keys by requesting the form. No matter what detection mechanism is used, I would like to see a MUST definition for the server to provide this mechanism. Without it, clients can only guess. 5. Section 4.2.2 reads: "it MAY omit all fields it does not know or where it does not change the value already supplied by the Search Service." This implies that the Search Service will use the default value of that field, if not supplied by the Searcher. If that's the intent, I'd also explicitly state that, to prevent confusion. I do wonder if that can cause weird issues. A searcher can submit the exact same search request (to different services) but it could represent a very different query. I wonder if that's going to cause problems at some point. The XEPs author replied via chat: "regarding the defaulting: you're right: absent fields should be interpreted as "client does not know them" as opposed to "use defaults"." Let's change the XEP to reflect this. 6. Section 4.2.2 reads: "The Searcher MAY include a Result Set Management (XEP-0059) [4] <set/>" Does this imply that the Service MUST support RSM? If that's not the case, then I would change 'MAY' for 'may' here. As XMPP is extensible, any entity should disregard extensions that it doesn't recognize anyway. With that in mind, the MAY usage seems to imply something. If that _is_ the case, then I'd prefer that to be explicitly stated (eg: "the Service MUST support RSM"). The XEPs author replied via chat: "yes, the service MUST support RSM. RSM is a hard dependency. (both sides MUST support RSM in fact, but the initial query does not necessarily need an RSM element, in which case page size etc. are defaulted by the service)." Again, I propose to make this explicit in the XEP. 7. Question on XEP-0433 / 4.2.2.6 (conflicting fields): Would usage of `all` combined with one of the vars that relate to `q` (like `sinname`) also be considered conflict to which a `<conflicting-fields/>` MUST be responded with? Is using `sinname` (and/or friends) _without_ `q` (or any equivalent, unspecified var) also considered a conflict? The XEPs author replied via chat: "yeah, I think `all` should conflict with all other fields which may restrict the resultset. `sinname` without a query or equivalent should also cause a conflict, yes." I answered: "even `all` combined with something like `sinname=false` ? Things get tricky there :) (and is that a MUST NOT or SHOULD NOT?) (which is increasingly annoying when the form is pre-populated by the server, as not supplying a field does not change the value already supplied by the Search Service. Doesn't this mean that the server can never pre-populate `sinname` and friends, as you can't un-populate those when you want to do a `all`?)" 8. example 10 https://xmpp.org/extensions/xep-0433.html#example-10 uses `not-allowed` while the text defines that `bad-request` should be used. 9. Table 1 defines `sinaddress` while example 3 uses `sinaddr` - muclumbus used `sinaddr` which makes this an easy one to get wrong, I think. Perhaps another item for a 'differences with precursor' section. 10. The XEP has "ignoring text search terms" in a couple of places that relate to `all`. I would remove that, as it implies that any value for `q` could be ignored (rather than that this is a MUST conflict scenario). 11. Section 6.2 describes the 'is-open' as boolean character data. "If set to true, it indicates that the channel can be joined without extra credentials." In examples, the element is used like this: `<is-open/>`. This to me seems to suggest that the element is present when the room is open, which suggests that the element is absent when the room is not. This does not track well with the definition, which seems to call for an explicit 'true' or 'false' value. 12. Somewhat of a nit, but table 2 doesn't define what the direction of the ordering is when using the address-based key. Consider making that more explicit. That's it, I think. Apologies for the haphazardous approach. This was compiled in many different sittings, which made the organisation go out of the window. - Guus