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

get_messages fails if any one of the messages does not fit the specification #108

Open
hoh opened this issue Sep 22, 2022 · 1 comment
Open
Labels
bug Something isn't working

Comments

@hoh
Copy link
Member

hoh commented Sep 22, 2022

There are some invalid messages in the Aleph Nodes, in particular PROGRAM messages found on https://api2.aleph.im/api/v0/messages.json?msgType=PROGRAM .

They fail being parsed as AlephMessage with either KeyError or pydantic.ValidationError.

With the new aleph-client only manipulating objects, this means that one invalid message fails the entire get_messages request.

async def get_messages(
    ...
) -> MessagesResponse:


class MessagesResponse(BaseModel):
    """Response from an Aleph node API."""

    messages: List[AlephMessage]

There are multiple ways to handle this:

  1. Ignore silently messages that do not validate
  2. Ignore with logging a warning messages that do not validate
  3. PyAleph APIs validate messages before returning them (or even drop them from DB)
  4. Return Messages mixed with Error objects

image

What do we want to go for ?

@hoh hoh added the bug Something isn't working label Sep 22, 2022
@odesenfans
Copy link
Contributor

Mmh conceptually I dislike the idea of the CCNs delivering data in the wrong format. But we have to consider two changes to the message spec:

  1. The message is incorrect, was integrated because of a bug in the CCNs and should be marked as invalid and discarded after an update fixes said bug
  2. The message is correct, an incorrect update to the message spec temporarily marks it as invalid. For example, this did happen when I introduced Pydantic models for validation on the CCN, my models were a bit too strict at the start.

Checking messages at runtime potentially has a huge cost, there are processes out there retrieving messages more than 10k at a time. Running Pydantic model validation on all these will definitely cause performance issues.

IMO the best solution would be to keep the DB clean: whenever we release a new version that touches message validation, a configurable mechanism should trigger a re-validation of all the messages stored on the node. This way, we could:

  • test the message validation on test nodes without affecting old messages
  • clean-up old messages once changes have been validated
  • have this without a runtime penalty on CCNs.

On the client side, we should definitely validate all the messages and print a warning if we detect invalid messages. So, basically, I'm all for option 2 (or 4, it's the same thing just a bit more dev-friendly) on the client-side, and a mechanism to re-validate all the messages on specific updates for CCNs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants