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

Improve the logic to extract the requestUserId for context objects #1147

Closed
1 of 6 tasks
seratch opened this issue Apr 28, 2023 · 5 comments · Fixed by #1375
Closed
1 of 6 tasks

Improve the logic to extract the requestUserId for context objects #1147

seratch opened this issue Apr 28, 2023 · 5 comments · Fixed by #1375
Assignees
Labels
enhancement M-T: A feature request for new functionality project:bolt
Milestone

Comments

@seratch
Copy link
Member

seratch commented Apr 28, 2023

While testing this new feauture, I found that the logic to set context.requestUserId can be improved for Events API patterns. I will make changes for it in a different pull request.

As I mentioned at #1144, bolt-java's user ID extration for context objects can be improved like bolt-python does.

Category (place an x in each of the [ ])

  • bolt (Bolt for Java)
  • bolt-{sub modules} (Bolt for Java - optional modules)
  • slack-api-client (Slack API Clients)
  • slack-api-model (Slack API Data Models)
  • slack-api-*-kotlin-extension (Kotlin Extensions for Slack API Clients)
  • slack-app-backend (The primitive layer of Bolt for Java)

Requirements

Please make sure if this topic is specific to this SDK. For general questions/issues about Slack API platform or its server-side, could you submit questions at https://my.slack.com/help/requests/new instead. 🙇

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you agree to those rules.

@seratch seratch added enhancement M-T: A feature request for new functionality project:bolt labels Apr 28, 2023
@seratch seratch added this to the 1.30.0 milestone Apr 28, 2023
@seratch seratch self-assigned this Apr 28, 2023
@seratch seratch modified the milestones: 1.30.0, 1.31.0 Jul 5, 2023
@seratch seratch modified the milestones: 1.32.0, 1.33.0 Sep 12, 2023
@seratch seratch modified the milestones: 1.33.0, 1.34.0, 1.x Oct 3, 2023
@lcf
Copy link

lcf commented Sep 15, 2024

Hi @seratch , do I understand correctly that userId is not extracted at all for events?

This means that installationService and distributed applications will never work with Slack Bolt for Java, because this method
public Installer findInstaller(String enterpriseId, String teamId, String userId) will never be called, since there was no userId extracted.

If that is true, is it okay to change this to a bug?

@seratch
Copy link
Member Author

seratch commented Sep 15, 2024

The request user ID is set for many request patterns, but currently, it's indeed unset for Events API request patterns. This lack of data affects your app's behavior only if your app manages a user token for all request users. If your app only manages bot tokens, the mandatory findBot() method works without any issues even for Events API patterns.

Regarding request user IDs in the Events API patterns, improving like bolt-python does not guarantee that the requestUserId will always be set for all request patterns. It's unclear which user ID should be picked for many events. Also, due to the complexity and variation of event payload data structure, the extraction logic cannot be perfect in a simple way.

For a specific use case requiring a user token, this may be called a bug, but we think this is a minor improvement. So, unfortunately we don't have immediate plans to work on it. If you're in a hurry to use a user token corresponding to a request, please consider adding additional code on your listener function side and call the findInstaller() method using the user ID to receive the user's user token.

It'd be appreciated if you could understand this.

@lcf
Copy link

lcf commented Sep 15, 2024

@seratch let's put user tokens aside. It does not work for many basic use cases as well. For example, a user opens home tab and we want to render some data for the current user. If we read ctx.getRequestUserId(), it is going to return null, even though:

  1. It is supported and works fine in other SDKs, like Java Script.
  2. We parse user id everywhere else in the Java SDK, just not for the events.

It is inconsistent, inconvenient and error prone.

We should copy these few lines: https://github.com/slackapi/bolt-js/blob/87d75c59f5eb9b28586173487dac1a0d6e1deada/src/App.ts#L1533

      if ('user' in event) {
        if (typeof event.user === 'string') {
          return event.user;
        }
        if (typeof event.user === 'object') {
          return event.user.id;
        }
      }

To EventRequest in Java SDK to fix the bug and achieve feature parity.
Do you agree @seratch ?

@seratch
Copy link
Member Author

seratch commented Sep 16, 2024

We parse user id everywhere else in the Java SDK, just not for the events.

No, you don't need to do. Regarding other request patterns apart from Events API ones (meaning block actions, view submissions, slash commands and others), the requestUserId is always available. Only Events API ones are unsupported at this moment.

We should copy these few lines: https://github.com/slackapi/bolt-js/blob/87d75c59f5eb9b28586173487dac1a0d6e1deada/src/App.ts#L1533

As I mentioned above, bolt-python's logic is better. So, we are planning to copy the logic in the future.

I don't say we won't implement this (that's why this issue is still open), but right now we don't have the bandwidth for this task due to priorities (we're focusing on Dreamforce etc.). If you need to write code immediately, please access the user ID via req.getEvent().getUser() in you AppHomeOpenedEvent listener. I don't think it's so bad, plus you can rely on the code forever (it's also fine to migrate to context#getRequestUserId() once bolt-java supports it soon).

seratch added a commit to seratch/java-slack-sdk that referenced this issue Sep 19, 2024
@seratch seratch modified the milestones: 1.x, 1.43.1 Sep 19, 2024
@lcf
Copy link

lcf commented Sep 23, 2024

Hey @seratch thanks for this!

If you need to write code immediately, please access the user ID via req.getEvent().getUser() in you AppHomeOpenedEvent listener. I don't think it's so bad, plus you can rely on the code forever (it's also fine to migrate to context#getRequestUserId() once bolt-java supports it soon).

having it context is much better. we have a more nuanced and structured application: certain classes only have access to the Context instance, and not the request itself, so it would have been mighty inconvenient to pass request object to those lower level classes, just for events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality project:bolt
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants