On 17.04.2017 15:32, Matthew Wild wrote:
> On 17 April 2017 at 13:43, Lennart Sauerbeck
> <de...@lennart.sauerbeck.org> wrote:
>> - Regarding the error handling in lines 49, 55 and in-tree 801 (the last
>> one is not in the patch because I did no changes there): Is it okay to
>> just send multiple error messages or should they be collected somehow
>> and sent as batch after the complete stanza was processed? I could find
>> nothing about this in either the MUC XEP, nor in the three (or so) base
>> RFCs of XMPP.
> Yeah, this is a problem. Of the three stanza types, changing
> affiliations uses <iq/> which is always request (get/set) -> response
> (result/error), and it's not acceptable to have multiple responses to
> a single request. Otherwise if you were the client, you wouldn't know
> when the responses had stopped coming, for example.

Ah, that makes sense. Thanks for the explanation.

> I don't have a good suggestion right now. There are a few solutions
> that come to mind:
>   - Always report success, just ignore errors. The client will see the
> affiliation change notifications generated by the room for the
> successful changes.

I like this approach, because it is easy and maintainable, but it might
not be the most intuitive for the user/client. How about a small change

- If everything could be applied as requested, report success.
- If at least one JID could not be handled successfully, apply what can
be applied and report an error with the malformed JIDs in the error message.

This might potentially lead to many requests resulting in errors.
However, the most intuitive as a user would be to simply retry -- most
clients would probably show the current affiliation list anyway and the
user would see which JIDs got applied and which did not.

>   - Treat the request as an atomic change: succeed for all, or fail
> all (i.e. if one of the JIDs is malformed, don't allow any of the
> other changes in the request to take place). This would require work
> to either pre-verify the changes (which we don't have code for) or to
> roll back changes that were already made (which is a bit hacky, we'd
> already have sent out notifications, unless we added a new 'batch
> update' version of :set_affiliation()).

Rolling back seems very hacky, I agree.

Regarding the pre-verifying: Malformed JIDs should be detected by
util.jid.prep(), right? And the other error cases should be handled in
room_mt:set_affiliation()? By extracting the error handling from
room_mt:sef_affiliation() it would be possible to do this early
verification. Basically, it would be just another loop over all given
JIDs. However, even if this would be the case, it still leaves open the
question of what to return.

However, the user may have spent a long time entering the JIDs (if there
are many), so ignoring all well-formed JIDs just because one was
malformed does not seem in the user's best interest, so I don't think
treating it as an atomic change is the way to go here.

Let me know what you would prefer and I'll give implementing it a try.

> Sounds great - ideally we'd have a version of the patch for trunk
> before merging, but let's keep the focus on 0.10 until we figure out
> the issue with the errors.

Sure, as soon as this patch is in an acceptable state I'll get working
on the trunk version with equivalent functionality.

Best Regards

You received this message because you are subscribed to the Google Groups 
"prosody-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to prosody-dev+unsubscr...@googlegroups.com.
To post to this group, send email to prosody-dev@googlegroups.com.
Visit this group at https://groups.google.com/group/prosody-dev.
For more options, visit https://groups.google.com/d/optout.

Reply via email to