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

Enforcing BlockKit limits #176

Open
bmorton opened this issue Aug 7, 2023 · 8 comments
Open

Enforcing BlockKit limits #176

bmorton opened this issue Aug 7, 2023 · 8 comments

Comments

@bmorton
Copy link
Contributor

bmorton commented Aug 7, 2023

In looking to replace our internal BlockKit implementation with this one, something that came up is that we'd lose the support we built for enforcing Slack's BlockKit limits on text fields and number of elements in various blocks.

Is this support that you'd be interested in accepting a pull request for? Do you have thoughts about how this should be implemented in this library?

@CGA1123
Copy link
Owner

CGA1123 commented Aug 8, 2023

Hey, this would definitely be a welcome addition of functionality to the ecosystem!

I would be interested in hearing about how you approached solving this internally and any strategies in particular for managing these validation failures (and if those have/would impact the design of any interface!).

It might be worth also exploring whether we can leverage existing tools to validate payloads -- rather than having to implement some kind of validation interface into this lib.

It looks like (although somewhat undocumented) it is possible to extract a JSON schema for block kit messages from their online block builder:

This could be functionality that lives within this library or independently as a "validator" for block kit JSON -- which may or may not have been generated via this gem 🤔

@bmorton
Copy link
Contributor Author

bmorton commented Aug 8, 2023

We've got a pretty straightforward system right now:

  • If you add an element to an array that exceeds max value, an exception is raised
  • If you try to create a field with a text field longer than allowed, an exception is raised

I agree that using the JSON schema would be the right way to do this. I took a swing at using that gist after digging out the non-truncated one that's buried in there. I couldn't get it to validate properly -- it seemed like it was only for a portion of the BlockKit spec or was too old to be useful, but I only took a pretty quick pass on it.

We've also reached out to Slack to see if they can provide this to us, but we haven't heard anything back yet.

If we were to do the former, I think it'd have to be embedded in the library. If we can take the schema-validator approach, I agree that we could have some broader utility if we keep it independent.

@CGA1123
Copy link
Owner

CGA1123 commented Aug 9, 2023

It does seem that maybe they are no longer exposing that schema anywhere anymore.

The current block builder UI now makes an RPC call to /api/blocks.format in order to run validation, rather than having any kind of client-side validation happening it seems.

If Slack don't provide a schema, I think it could be beneficial to scrape one together and keep it up to date over time, especially if it is in a language agnostic form.

To some extent, if there were an exposed schema, most of the code currently in this gem could simply be generated, which might be quite nice and guarantee a level of consistency in the API, depending on how nice the generator was!

@bougyman
Copy link

bougyman commented Oct 4, 2023

Would be willing to help move this forward, is there a working branch/fork available?

@lritter
Copy link

lritter commented Nov 15, 2023

Hey there. I sketched out a possible approach here #180 and would love to hear if this might help. The PR adds the ability to flexibly check for limit/schema violations (though doesn't use json schema but rather just checks limits advertised in the documentation) as well as sanitize the json produced during serialization. This allows for, say, option labels to be truncated rather than raising an error. You could imagine implementing your own "limiter" to additionally provide tracing or logging in such cases.

This is just a sketch to illustrate a few different scenarios but I'd love feedback on whether this seems useful. Thanks!

@CGA1123
Copy link
Owner

CGA1123 commented Nov 21, 2023

Hey @lritter -- thanks for taking the time to look at this and for providing a potential solution. Much appreciated!

I apologise for the delayed reply. I haven't yet been able to make the time to think this through in full, but wanted to ACK this and will do my best to make some time in the evening this week or over this coming weekend.

In the meantime, would be interested to hear from @bougyman + @bmorton about thoughts and whether this would cater to your use-cases -- you are better placed than I am to shed light on clear requirements/wants from a solution!

@lritter
Copy link

lritter commented Nov 21, 2023

Appreciate it @CGA1123!

I'll say that one of the things I don't like about my solution is that it mixes the "validation" concern with the "make it work" concern (modifying supplied values such as long labels to allow things to work). Doing the "make it work" stuff at the application layer makes sense but being able to hook into the framework to do this greatly eases development and reduces the chance of missing stuff. The PR here seems like "the simplest thing that could work" but I'm definitively curious about other options.

Thanks!

@CGA1123
Copy link
Owner

CGA1123 commented Nov 27, 2023

I think the idea of a serialisation middleware stack is slick, and allows for all sorts of flexibility!

There's then probably a set of common implementations for field types that have common restrictions (text length, enum types, etc...) which probably need to have some different ways to "fail": notify, raise, "try to autofix" (e.g. truncate).

I like this because it also keeps the door open for curating or finding a JSON-Schema based approach for all these components, and potentially leveraging existing tooling for validation around that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants