search

tigase / Martin

(M) Martin - XMPP client library for Swift
Other
69 stars 27 forks source link

Fix Element equatable implementation #7

Closed tapsandswipes closed 3 years ago

tapsandswipes commented 3 years ago

If the number of nodes is the same the comparison returns true even if the content is not equal

Signed-off-by: Antonio Cabezuelo Vivo antonio@tapsandswipes.com

hantu85 commented 3 years ago

This implementation of equality check is intentional and designed to be fast as it often called and recursive comparison done on all children of the element would add a significant penalty.

However, this was an undocumented behaviour and could look like an error. I've added a documentation for this method explaining its behaviour.

tapsandswipes commented 3 years ago

The problem is that, in swift, == means equal. If you need an implementation that can fail you should use another method with proper naming, because providing wrong implementations for standard methods can lead to errors very difficult to debug. I've lost many hours till I found this one.

woj-tek commented 3 years ago

@tapsandswipes Out of curiosity: could you give some XMPP stanza/payload (ideally with XEP that specifies it) example where this yielded the error? (would be easier to address)

tapsandswipes commented 3 years ago

I was trying to generate a diff between 2 elements for a custom iq stanza:

<item><name>Antonio</name></item> == <item><name>Antonio Cabezuelo</name></item>

returns true

hantu85 commented 3 years ago

@tapsandswipes I understand that, but Swift left implementation of objects equality check to developers as only they know what they "assume" as equal. Moreover, Swift uses == method to equality checks when comparing items within a collection and find item position or removal of item from a collection. Doing full equality check for that will add a significant penalty.

Due to that I've reviewed my position, and will update == method to do a full equality check. However, I'm going to update removeChild() method to use reference comparison while looking for element to be removed. I think that this could improve performance and will not cause a full equality check and removal of only instances of Element object which are actually there is a good idea.

Additionally, Element (while still containing == method) will not implement Equatable protocol. This will be done, to make sure that people trying to remove an element from a collection will actually use a correct methods for removal instead of using methods which could allow passing an Element and getting penalty of doing a full equality check.

hantu85 commented 3 years ago

The fix for the issue has been applied.