Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify schema validation #684

Merged
merged 20 commits into from
Feb 18, 2022
Merged

Unify schema validation #684

merged 20 commits into from
Feb 18, 2022

Conversation

silverbucket
Copy link
Member

@silverbucket silverbucket commented Feb 11, 2022

This turned in to quite a large PR, there are going to be breaking changes specifically with the format of some of the messages sent to the client during an IRC session, and also maybe some things related to presence.

  • Move all schema validation logic and handling to sockethub-schemas
  • sockethub-schemas exports helper functions for validating AS objects for use in tests or on the fly
  • irc2as has unit tests to validate all of it's generated AS objects which are sent to the client
  • sockethub hands off all validation to sockethub-schemas
  • sockethub-schemas has near 100% test coverage
  • sockethub-schemas has re-written logic for finding the best error message from the list of ajv errors when a schema fails validation. This is still not perfect, but much better than it was.

Resolves #681

@silverbucket silverbucket added type:bug state:ready kredits-2 Medium contribution type:technical-debt package:core Issues related to Core Sockethub package package:schemas Issues related to Schemas package:irc2as Issues related to IRC2AS info:breaking-change labels Feb 11, 2022
@silverbucket silverbucket added this to the Hyperchannel Alpha milestone Feb 11, 2022
@silverbucket silverbucket self-assigned this Feb 11, 2022
@raucao
Copy link
Contributor

raucao commented Feb 11, 2022

Wow, large indeed!

there are going to be breaking changes specifically with the format of some of the messages sent to the client during an IRC session, and also maybe some things related to presence

What exactly are the breaking changes? It takes much more time to adjust client code without knowing what needs adjusting.

Copy link
Contributor

@raucao raucao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment about spec usage/wording for now. Have yet to test with Hyperchannel.

packages/irc2as/src/index.test.js Outdated Show resolved Hide resolved
@silverbucket
Copy link
Member Author

silverbucket commented Feb 11, 2022 via email

@silverbucket
Copy link
Member Author

@raucao I've refactored the getErrorMessage() method to satisfy codeclimate, and made a best effort to improve the irc2as unit test structure. This is ready for review.

@silverbucket
Copy link
Member Author

To summarize some of the breaking changes with objects coming from irc2as:

  • errors were sometimes represented as a type in the object, this has been changed to the error property (string) on the root of the AS object, as was settled on in sockethub core.
  • actor and target have been swapped in the case that an action was an error. The actor is now the person who intiated it, rather than before where it was represented as the room (actor) sending an error to the user (target)
  • context irc was added to all object
  • send was changed to update for errors specifcially.
  • the topic object uses the standard content property for the topic, rather than a topic property.
  • MOTD was changed to a long string thats appended per-line of irc input, rather than an array of lines one for each input.

That about sums up the breaking changes for incoming IRC AS to the client. There are likely a few other breaking things in here but hopefully they'll be very easy to identify (esp. considering the schema errors have been improved). This should be a major step towards tracking future breaking changes though, since we're validating everything upfront now.

@raucao
Copy link
Contributor

raucao commented Feb 13, 2022

I'm able to reproduce #679 with this branch (and 67P/hyperchannel#282), when just opening Hyperchannel and have it connect to a saved XMPP account and join a couple of rooms. It only happens when actually connecting from Sockethub though, not when reloading Hyperchannel, and thus using the same connection and presence.

The Sockethub console logs also show separate presence updates for every person a room, and falsely call them "contacts" (a term that should be reserved for full JIDs that are actually in your roster, as opposed to a nickname in a public room):

  sockethub:platform:xmpp:380ee2e got room attendance list +3ms
  sockethub:platform:xmpp:380ee2e received contact presence update from kosmos-random@kosmos.chat +1ms
  sockethub:platform:xmpp:380ee2e received contact presence update from kosmos-random@kosmos.chat/raucao +0ms
  sockethub:platform:xmpp:380ee2e received contact presence update from kosmos-random@kosmos.chat/jimmy +1ms

Edit: when reloading, it will only send the normal attendance list with members inside of the message's object. So I guess the XMPP service may actually send separate presence messages for what may be members of a room, or for actual roster contacts being present in a room. In which case the only thing wrong would be that the room itself is still typed as a person. (However, I don't think I ever added Jimmy as a member in kosmos-random@kosmos.chat.)

@silverbucket
Copy link
Member Author

@raucao OK, I've added schema validation tests for output XMPP AS objects as well, to further tighten things up. So the objects should all be in a unified format now. The only remaining thing is with guessing the type... right now the guessing is person but we need some way of determining when to set type room instead. I could use your help on that as I don't have any ideas off the top of my head.

@raucao
Copy link
Contributor

raucao commented Feb 15, 2022

There's no way of guessing type room from nothing but a JID string, because they look exactly the same as a person.

I guess the cheapest way to determine if a JID belongs to a room is to know beforehand which domains belong to a MUC service. For example when a join is received, it could remember the MUC domain as such.

Another way would be to actually discover the MUC domain in the process; something we need a message/function for anyway, to allow users to discover their own server's MUC. This could, and maybe should, be combined with discovering and caching the other information about the service in one go, because the MUC info is requested and received in a generic disco#items request:

https://xmpp.org/extensions/xep-0045.html#disco-service

(As for Git and Kredits hygiene, I think that should be done in a separate PR, either to this branch, or to master after merging this.)

@raucao
Copy link
Contributor

raucao commented Feb 15, 2022

... btw, I think #679 is low priority, as it's not even used in Hyperchannel at the moment, and also not in other code I'm aware of. And the fix also wouldn't really break anything, so it can be done after a major release, and maybe in the same go as adding more discovery features to the XMPP platform.

@silverbucket
Copy link
Member Author

@raucao OK, I've updated the ticket to not resolve #679 ... barring that, does the PR work for you?

Copy link
Contributor

@raucao raucao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@silverbucket silverbucket merged commit 3c8bebc into master Feb 18, 2022
@silverbucket silverbucket deleted the unify-schema-validation branch February 18, 2022 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info:breaking-change kredits-2 Medium contribution package:core Issues related to Core Sockethub package package:irc2as Issues related to IRC2AS package:schemas Issues related to Schemas state:ready type:bug type:technical-debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate outgoing AS objects
2 participants