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

Add support for remote functions #2026

Merged
merged 17 commits into from
Aug 14, 2024
Merged

Add support for remote functions #2026

merged 17 commits into from
Aug 14, 2024

Conversation

misscoded
Copy link
Contributor

@misscoded misscoded commented Jan 8, 2024

Summary

This PR adds support for remote (i.e., remotely hosted) functions for use in Workflow Builder.

What's New

A shiny new listener: .function()

Support of remote functions brings a new .function() listener that features complete and fail utilities to quickly and easily signal if the function has successfully executed or not.

app.function('sample_function', async ({ inputs, complete, fail }) => {
  try {
    const { sample_input } = inputs;
    complete({ outputs: { sample_output: sample_input } });
  } catch (error) {
    console.error(error);
    fail({ error });
  }
});

Function-specific interactivity support

We've beefed up the existing action listener to automatically determine if the action/interactivity event being received is associated with a function. If so, the callback will make available the complete and fail utility functions. Note: this requires that the interactivity call was made with the incoming JIT token. See below for details.

app.action('sample_button', async ({ ack, context, complete, fail }) => {
  await ack();
  const { functionExecutionId } = context;

  try {
    complete({ function_execution_id: functionExecutionId });
  } catch (error) {
    console.error(error);
    fail({ error });
  }
});

Automatic use of JIT token (with opt-out option)

When a function-related event is received, a JIT token is included in the payload. The subsequent use of this token when making API calls is what allows for interactivity to be associated with that function (and eventually see the function through to complete or fail). Out of respect for all of our developers and their individual setups, we've provided an easy way to opt-out of this JIT token attachment with the attachFunctionToken property in AppOptions.

const app = new App({
  token: process.env.SLACK_BOT_TOKEN,
  socketMode: true,
  appToken: process.env.SLACK_APP_TOKEN,
  logLevel: LogLevel.DEBUG,
  // To opt-out of using the JIT token to make `client` calls in
  // function-related callbacks, set attachFunctionToken to false.
  // attachFunctionToken: false,
});

Outstanding / To Do

  • DRY-up complete/fail utility logic (App.ts / WorkflowFunction.ts)
  • Shore up TS (specifically, review events)
  • Tests

Requirements (place an x in each [ ])

@misscoded misscoded added enhancement M-T: A feature request for new functionality semver:minor labels Jan 8, 2024
Copy link
Contributor

@WilliamBergamin WilliamBergamin left a comment

Choose a reason for hiding this comment

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

This is awesome 💯 first pass everything looks good! I left some comments on naming alignment

src/WorkflowFunction.ts Outdated Show resolved Hide resolved
src/App.ts Outdated Show resolved Hide resolved
src/App.ts Outdated Show resolved Hide resolved
src/types/events/base-events.ts Outdated Show resolved Hide resolved
src/WorkflowFunction.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Looking good!

One higher-level API that bolt needs to consider (and that the deno SDK does not) is token selection when creating the helper API client and other convenience utility functions like say. Some bolt apps implement OAuth and ask for user scopes, so a user token may come into play. While the function complete/fail helpers should always use the JIT / workflow / function-execution-specific token, that is not so clear for say or for the general API client available in certain event handlers. We need to think about how to expose the option of which token to use in these situations.

src/App.ts Outdated Show resolved Hide resolved
src/WorkflowFunction.ts Outdated Show resolved Hide resolved
src/WorkflowFunction.ts Outdated Show resolved Hide resolved
src/WorkflowFunction.ts Outdated Show resolved Hide resolved

function selectToken(context: Context): string | undefined {
// If attachFunctionToken = false, fallback to botToken or userToken
return context.functionBotToken ? context.functionBotToken : context.botToken || context.userToken;
Copy link
Contributor

Choose a reason for hiding this comment

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

To @WilliamBergamin's earlier point, this area may need to be exposed as an API to the dev. Here we are making an assumption that for non-function-scoped events, the dev will want to use a bot token over a user token - that will not always be the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how WorkflowStep was implemented, so merely a carry over. That was long ago, but is there a reality where an app has both present -- a bot token and a user token? In my mind, I've always thought of it as one or the other (either it's a bot, or an app acting on behalf of the user), but you bringing this up makes me think that's a faulty mental model.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, an app can ask for scopes of both types (if you head to the OAuth & Permissions app config page, you can subscribe to both).

Just this week I was helping someone who was using a user token for one API (uploading files) but a bot token in another (posting a message containing the uploaded file).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, for any event that currently comes in, when we run selectToken when that event is processed, we opt for botToken first, followed by userToken. I think it's always been this way, or has for the last 4+ years.

@seratch your name is attached to one of those implementation lines. Can you advise here?

Copy link
Contributor

@filmaj filmaj Jan 19, 2024

Choose a reason for hiding this comment

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

I've always found this area to be poorly documented on our part and glossed over. For example bolt-js only mentions user scopes if you REALLY dig deep: you gotta scroll to the end of the OAuth docs, expand the hidden "Customizing OAuth options" section and then piece together the pieces based on the example and the one-line description of the userScopes field.

I believe, but may be wrong on this, that this has always been a userland concern that we don't provide guidance on. Like, for an app that is installable on-demand to workspace (implements OAuth) and collects both bot and user tokens, I'm pretty sure it is up to the installation store implementation (so userland code) to both: figure out which token to store as well as which token to select and return when doing the 'authorization' process that bolt exposes APIs for.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I overlooked this thread last week. The logic to select a primary token for context object from bot / user tokens is by design from the beginning, and all Bolt frameworks align.

We basically encourage developers to have a bot token, but technically it still should be allowed to go with only a user token (I don't think this is a common use case though). Thus, when a bot token is missing, setting a user token instead is a reassonable design for supporting such a scenario.

I don't think bolt should expose a way to customize this because:

  1. we've never received such feature request in the last few years because most apps have their bot token
  2. developers still can accesss both a bot and user token

As for the documentation topic, indeed this could be improved in the advanced topic section although the number of devs who need it is not so large.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK good to know and thanks for that context Kaz 🙇
If Kaz says so then I defer to his suggestion / feel free to ignore my point

src/WorkflowFunction.ts Outdated Show resolved Hide resolved
src/types/events/base-events.ts Show resolved Hide resolved
Copy link

codecov bot commented Jan 26, 2024

Codecov Report

Attention: Patch coverage is 76.14679% with 26 lines in your changes missing coverage. Please review.

Project coverage is 81.59%. Comparing base (01c174d) to head (0110bf2).
Report is 1 commits behind head on main.

Files Patch % Lines
src/App.ts 18.51% 15 Missing and 7 partials ⚠️
src/CustomFunction.ts 95.38% 0 Missing and 3 partials ⚠️
src/middleware/builtin.ts 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           main    #2026       +/-   ##
=========================================
+ Coverage      0   81.59%   +81.59%     
=========================================
  Files         0       19       +19     
  Lines         0     1646     +1646     
  Branches      0      464      +464     
=========================================
+ Hits          0     1343     +1343     
- Misses        0      194      +194     
- Partials      0      109      +109     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Great work! Left a few comments


// Making calls with a functionBotAccessToken establishes continuity between
// a function_executed event and subsequent interactive events (actions)
const client = new WebClient(token, { ...functionArgs.client });
Copy link
Member

Choose a reason for hiding this comment

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

This is the first time to see this way to initialize a new instance. Other code in bolt-js uses clientOptions for the second argument instead. I haven't verified on my end yet but have you confirmed this code can make the same effect with new WebClient(token, clientOptions)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm 99% sure I tested this when you brought it up as a concern back in January, but we will make doubly-sure before we cut the stable release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Going to take a slightly different tact here: expose the web client options as assembled in the bolt App constructor via a getter method, and simply re-use those here instead.

src/types/events/base-events.ts Show resolved Hide resolved
id: string;
callback_id: string;
title: string;
description: string;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description: string;
description?: string;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Also included in pre-stable-fixes branch.

Comment on lines +191 to +192
* 1. removes the next() passed in from App-level middleware processing
* - events will *not* continue down global middleware chain to subsequent listeners
Copy link
Member

Choose a reason for hiding this comment

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

🤔 Is removing this needed for correct function handling? I can imagine adding middleware after the function listener handler to do something specific to functions after some response like so:

app.function("example_callback", filterInputs, async ({ inputs, complete, next }) => {
  await complete({ outputs: inputs });
  await next();
}, saveResponseToDatabase);

It's not super clear that this is possible in docs - looking into this now - but I'm finding that listener middleware can go in any order, unrelated to the main listener handler.

Things do get interesting if functionBotAccessToken: true is used in that example though since the attached xwfp token is revoked soon after complete, meaning saveResponseToDatabase will get a token_revoked error if it tries using it...

But I think that error is better than swapping for the original bot or user token too since those tokens can be found again. I'm curious about the middleware handling most though! Leaning towards having processMiddleware handle callbacks and middleware without change if that's possible 👀

Copy link
Member

Choose a reason for hiding this comment

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

Global middleware should also run before these listeners too, but app.error might catch thrown errors if that happens. Thinking this is all fine as is!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This behavior works as expected, and is modeled off of the original WorkflowStep class. next is still available for developers to use within the callback in the case of additional middleware that they've included, but it's removed here so as not to allow the event to fall further down.

Admittedly, this is overkill, as WorkflowStep featured events that could be handled by more than one handler (ie, view_submission). We don't have a downside of keeping it, and it makes it extensible if and when there is overlap, but if folks feel strongly about it, we can remove it (though it will need to be re-tested).

zimeg and others added 2 commits June 19, 2024 11:53
@filmaj filmaj marked this pull request as ready for review August 12, 2024 19:30
@misscoded misscoded requested review from seratch and zimeg August 13, 2024 16:18
@filmaj filmaj merged commit 88e897a into main Aug 14, 2024
12 checks passed
@filmaj filmaj deleted the feat-functions branch August 14, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:signed enhancement M-T: A feature request for new functionality semver:minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants