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 though? - 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 Lennart -- 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.
