jdev - 2020-02-12

I feel like I've had this discussion a few times, but I never remember: if I get an IQ but don't recognize the local name (but do recognize the namespace) should I send back bad-request or feature-not-implemented? conversations.im (ejabberd, I think) sends back bad-request, which makes sense because it doesn't match the schema, but feature-not-implemented seems to overlap and be more specific. Maybe it doesn't matter at all?

Sam Whited, your description is basically identical to the text description of feature-not-implemented, so, well, that might be suitable :)

Seems more or less identical to the description of bad-request too though (it obviously doesn't match the schema for that particular namespace/feature), and that's what ejabberd returns so I was wondering if there's a reason

Prosody matches most iqs on the (name xmlns) tuple, then says service-unavailable if nothing handles it

I send back service-unavailable, because I don’t distinguish "knowing the namespace" vs. "knowing the namespace and the local-name"

Oh geeze, a third one with overlap. That would be a lot easier actually, I had been planning on sending that back if the namespace wasn't recognized, so maybe I'll just send one error and call it a day.

Doesn't sound like there's any weird compatibility issues with one vs another anyways, so I might as well start with service-unavailable and then if users complain I can add more fine grained error handling later

I stumbled upon XEP-0045 saying that service-unavailable (and type=wait) means that the room "has reached its maximum number of occupants"

So, everything is a mess and nothing means anything, do whatever you want

*sigh* that figures. Thanks for the help

I can't imagine what the user would do with the knowledge that we do handle the namespace but not the localname anyways, so just having a generic error is probably good enough

Guess what Prosody says if an external Component isn't connected?

service-unavailable type='cancel'?

type=wait

close.

Maybe that should be one of the remote-server-somethings

This led to a client saying "Room is full" when my Biboumi instance wasn't started

Much fun!

ejabberd also includes "code='503'"… might just pretend I didn't see that, I really hated the stupid HTTP-like error codes.

Pretty sure those are deprecated since decades

yah, if someone complains they can implement it later.

... Except in MUC!

I assume Cisco or Google or someone still requires them and someone will eventually complain.

Can't remember any complaints about Prosody not sending them

Good to know, thanks

Sam Whited, was your original question about a client(-library)?

A library, it could be a client or server, just for anything receiving an IQ it doesn't recognize.

I think it's best to just return service-unavailable as a generic-catch-all

Yah, I'll do that and type='cancel' so make it clear that they shouldn't retry in this case. Thanks.

In the case of connected clients, it makes it a bit harder to identify connected resources. Tho you can probably tell from timeing anyways.

In this case if you were writing a server with this library that would already be handled for you elsewhere, luckily. Can't remember what I send back for that. This fallback is just for user registered handlers that got routed correctly, but now we don't recognize the IQ payload.

Trying to figure out a good API for registering handlers for different IQ payloads but I just realized my design was assuming 0 or 1 child elements, which is only true for result="true" IQs. I suppose I could match each separate child element in the IQ to a handler, but I bet if you're including multiple things in an IQ they need the context of all the other child elements too.

And I don't want to decode the entire thing or do any sort of complex query language if I can avoid it; this was supposed to be a bare-bones dead simple way to call events based on elements, fancier stuff can be written by people using the library if they want it, the built in way should just be simple.

IQ queries must have exactly one child, responses and errors can have 0 to 2 childs

get/set = 1, result = [01], error = [12], from memory.

That's what I had thought, but I couldn't find it in RFC 6120… just did though.

Thanks, *whew* I was scared I'd have to redo all of that.

Not sure what I'll do about errors, but I might just ignore the child element from the get or set

If someone actually wants that payload for some reason they can write their own multiplexer.

or router or whatever this should be called in XMPP land.

Actually, no, this doesn't have to care about errors since those are already sent in response to something else and those are automatically returned to the user from the function they used to send the original IQ, not handled as incoming IQs.

So that all works out nicely and I just need more coffee I guess.

It's almost always sufficient to route iqs based on name+space of the primary child for get/set, and based on id/from for error/response.

Yah, that was a much better way to say it, thanks. Doing that.

Some basic dispatch based on iq type first maybe

Sorry, yes, I meant type+name+space.

I'm ignoring type actually (except that get/set and error/response are handled in different places as noted)

I wouldn't, FWIW, merge get/set.

yah, I just hand that off to the user; chances are they're going to handle it in the same place with an if statement.

My experience (with implementations that do that) is that it's unhelpful, but YMMV. ✏

You mean that it's unhelpful to have separate handlers for get/set, or unhelpful to just hand incoming IQs off to the user and let them decide if it's a get or a set?

It's unhelpful to have to start all handlers with if (get)...

In Prosody many of the ones that are split in get and set have a bunch of code overlap, tho it feels safer somehow.

It's not a major wart, but as every handler of which I'm aware needs different behaviour for get and set, it forces every handler anywhere to have the if(get)... scaffolding in it.

And smart money says if you write enough of those, you'll flip the logic for a fun bug some day.

Not that I have any experience of that...

And then one day someone forgets to add that, leading to handling get and set the same way, leading to other things starting to rely on that, leading to a mess.

So I'd gently encourage splitting of get/set handlers for the sake of your consumers, and then go slink back off to the shadows.

Interesting; I'll think about it.

I'd think they'd share so much logic that you'd want the if get/set instead of having a bigger list of handlers to register, but I defer to experience over me just kind of guessing about it

Don't think I've seen /that/ much shared logic

Actually, yah, on second thought I bet 90% of handlers are just "if get dbLookup else dbExec"

Maybe it just comes down to whether an if/else or registering two handlers is clearer or requires more boilerplate

I can imagine that some would share some auth checks

I think it both comes down to which is clearer, and which is more likely for someone to slip up.

If you have to register get/set explicitly it's impossible to forget to do so (although you can flip them). If you have to `if` in the call, it's possible to not only flip them, but cause some or all of the code to execute for both when you didn't intend it.

That makes good sense

More explicit to have shared/common code factored out in a function