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 in-band syncing #702

Merged
merged 22 commits into from
Oct 21, 2024
Merged

Add support for in-band syncing #702

merged 22 commits into from
Oct 21, 2024

Conversation

jpwilliams
Copy link
Member

@jpwilliams jpwilliams commented Sep 7, 2024

Summary

Adds in-band sync support by returning different data based on whether a PUT request is signed (and validated) or not.

In draft while functionality is built out elsewhere.

Checklist

  • Added a docs PR that references this PR N/A Accounted for elsewhere
  • Added unit/integration tests
  • Added changesets if applicable

Related

@jpwilliams jpwilliams added the ✨ new New features, integrations, or exports label Sep 7, 2024
@jpwilliams jpwilliams requested a review from amh4r September 7, 2024 12:32
@jpwilliams jpwilliams self-assigned this Sep 7, 2024
Copy link

changeset-bot bot commented Sep 7, 2024

🦋 Changeset detected

Latest commit: 86f259e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
inngest Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@inngest-release-bot inngest-release-bot added the 📦 inngest Affects the `inngest` package label Sep 7, 2024
@jpwilliams jpwilliams marked this pull request as ready for review September 9, 2024 11:47
@@ -756,7 +760,7 @@ export class InngestCommHandler<
}),
methodP,
methodP.then((method) => {
if (method === "POST") {
if (method === "POST" || method === "PUT") {
Copy link
Member Author

Choose a reason for hiding this comment

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

PUT requests can be signed now, so we make sure to access the body here. Some frameworks will throw errors if you attempt to access the body of a request that cannot have one (like a GET), hence abstracting it here.

Comment on lines +1149 to +1151
Promise.resolve(
parseAsBoolean(this.env[envKeys.InngestAllowInBandSync])
)
Copy link
Member Author

Choose a reason for hiding this comment

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

Only consider in-band syncs if we're explicitly allowing it, otherwise always fall back to out-of-band.

// Validation can be successful if we're in dev mode and did not
// actually validate a key. In this case, also check that we did indeed
// use a particular key to validate.
const sigCheck = await signatureValidation;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is always true if the SDK's mode is "dev", but will fail if it's "cloud" and the request could not be validated.

Comment on lines +1173 to +1175
if (!sigCheck.success) {
return {
status: 401,
Copy link
Member Author

Choose a reason for hiding this comment

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

A failed validation when we are allowing an in-band sync and one has been requested is always a 401.

packages/inngest/src/components/InngestCommHandler.ts Outdated Show resolved Hide resolved
@jpwilliams jpwilliams merged commit 4df5a01 into main Oct 21, 2024
47 checks passed
@jpwilliams jpwilliams deleted the feat/in-band-sync branch October 21, 2024 15:45
jpwilliams pushed a commit that referenced this pull request Oct 21, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## [email protected]

### Minor Changes

- [#702](#702)
[`4df5a01`](4df5a01)
Thanks [@jpwilliams](https://github.com/jpwilliams)! - Add support for
in-band syncing

### Patch Changes

- [#721](#721)
[`59fa466`](59fa466)
Thanks [@jpwilliams](https://github.com/jpwilliams)! - Support Next.js
15 in serve handler typing

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 inngest Affects the `inngest` package ✨ new New features, integrations, or exports
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants