search

laserdisc-io / slack4s

A functional scala library for easily constructing slack bots
MIT License
4 stars 2 forks source link

Slash commands with validated signatures are sometimes null #77

Closed jarrodcodes closed 1 year ago

jarrodcodes commented 1 year ago

Sometimes Slack commands that are received that pass signature validation still do not parse fully, with the SlashCommandPayload model containing null for certain required fields, such as command text or user id. This is a random and intermittent occurrence, and hard to replicate. However, I have traced the issue down to the library using the native Java parser SlashCommandPayloadParser which unfortunately allows nullability on all fields. This results in a valid slash command as far as this library is concerned but that contains multiple nulls. But according to the Slack API documentation, these fields should be populated, so we should fail the request and not allow this to occur.

Example parsed result log:

Screenshot from 2023-02-16 15-14-12

There are two solutions that would make sense to me:

  1. Switch to a native http4s form decoder for SlashCommandPayload that disallows nulls. The benefits to this option are that we will have no nullability and nicer optics around failures and decoding results. The cons to this option are that we now have to track changes to the SlashCommandPayload model and risk drifting compatibility. The risk of this seems quite small considering we are already pegged to the Java library's version that tracks the Slack API.
  2. Continue to use the native Java SlashCommandPayloadParser but validate it's output using Validated. The benefit of this approach is that we can continue to track API changes without the effort and implementation of our own decoder. I have also never seen partial failures, but only total failures, so checking even one essential field, such as the command field, should be sufficient. The con is that this requires a bunch of extra validation logic that could come for free from the first option, especially if we wanted to validate every field.
barryoneill commented 1 year ago

Oh wow - thank you for the effort!

The SlashCommandPayload hasn't changed much in the time I've used it, so I'm not too worried about API drift. I always lean towards stronger typing, so option #1 appeals to me more.

barryoneill commented 1 year ago

@jarrodcodes 1.1.0 has just been published, it should hopefully show up in maven central in a short while.

Thank you for the contribution!

jarrodcodes commented 1 year ago

Thank you very much, awesome!