-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Fix #18220 (airtable_oauth): normalize fallback events, add retry #18221
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,5 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import common from "./common-webhook.mjs"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { withRetry } from "../../common/utils.mjs"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export default { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ...common, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -10,10 +11,19 @@ export default { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async emitEvent(payload) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const changed = payload?.changedTablesById; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const tableEntry = changed && Object.entries(changed)[0]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!tableEntry) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Unknown / empty shape — emit normalized raw so consumers still get a consistent shape | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.$emit({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| originalPayload: payload, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, this.generateMeta(payload)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tableId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tableData, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ] = Object.entries(payload.changedTablesById)[0]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ] = tableEntry; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| operation, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| recordObj, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -52,11 +62,24 @@ export default { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ? "created" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| : "updated"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { fields } = await this.airtable.getRecord({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| baseId: this.baseId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tableId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| recordId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let fields = {}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const res = await withRetry( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| () => this.airtable.getRecord({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| baseId: this.baseId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tableId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| recordId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fields = res?.fields ?? {}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (err) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+65
to
+76
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Refine retries to avoid useless 4xx retries and add jitter Pass - try {
- const res = await withRetry(
- () => this.airtable.getRecord({
- baseId: this.baseId,
- tableId,
- recordId,
- }),
- );
+ try {
+ const res = await withRetry(
+ () => this.airtable.getRecord({
+ baseId: this.baseId,
+ tableId,
+ recordId,
+ }),
+ {
+ retries: 2,
+ baseDelay: 500,
+ // Retry on 5xx / 429 / network only
+ shouldRetry: (err) => {
+ const status = err?.statusCode ?? err?.response?.status;
+ return status == null || status >= 500 || status === 429;
+ },
+ },
+ );📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.log("Airtable getRecord failed; emitting empty fields", { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| statusCode: err?.statusCode ?? err?.response?.status, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| message: err?.message, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fields = {}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const summary = `Record ${updateType}: ${fields?.name ?? recordId}`; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -134,8 +134,12 @@ export default { | |||||||||||||||||||||||||||||||||
| return this.changeTypes; | ||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||
| emitDefaultEvent(payload) { | ||||||||||||||||||||||||||||||||||
| // Normalize the fallback so consumers always get a consistent structure | ||||||||||||||||||||||||||||||||||
| // matching our other emitters (which include originalPayload). | ||||||||||||||||||||||||||||||||||
| const meta = this.generateMeta(payload); | ||||||||||||||||||||||||||||||||||
| this.$emit(payload, meta); | ||||||||||||||||||||||||||||||||||
| this.$emit({ | ||||||||||||||||||||||||||||||||||
| originalPayload: payload, | ||||||||||||||||||||||||||||||||||
| }, meta); | ||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+137
to
143
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainPotential breaking change: default emission now wrapped; provide BC path Wrapping the payload under - const meta = this.generateMeta(payload);
- this.$emit({
- originalPayload: payload,
- }, meta);
+ const meta = this.generateMeta(payload);
+ // Back-compat: spread raw keys for existing consumers; new consumers use originalPayload.
+ this.$emit({
+ originalPayload: payload,
+ ...payload,
+ }, meta);If you prefer a hard switch, gate the spread behind an env flag (e.g., 🏁 Script executed: #!/bin/bash
set -e
echo "Listing files in components/airtable_oauth/sources/common:"
ls -1 components/airtable_oauth/sources/common
echo
echo "Inspecting all $emit calls in common directory:"
rg -n "\$emit" -C1 components/airtable_oauth/sources/common/*.mjs || true
echo
echo "Inspecting originalPayload usage in common directory:"
rg -n "originalPayload" -C1 components/airtable_oauth/sources/common/*.mjs || trueLength of output: 3278 Provide backward-compatibility by dual-emitting raw payload fields The new emitters now nest the entire payload under Update these emit calls:
Example diff for common-webhook.mjs: - this.$emit({ originalPayload: payload }, meta);
+ this.$emit(
+ { originalPayload: payload, ...payload },
+ meta
+ );If you prefer a toggle, wrap the spread in an env-flag check (e.g. 📝 Committable suggestion
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't that what versioning is for?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mariczne You're absolutely right! Versioning is the cleaner approach here. If you're planning to bump the component version for this structural change, then the clean payload wrapping under A version bump would:
Are you planning to version this as a major update? If so, the current implementation is much better than my suggested dual-emission approach. 🤔 Hmm, you make a good point about versioning... 🧠 Learnings used |
||||||||||||||||||||||||||||||||||
| async emitEvent(payload) { | ||||||||||||||||||||||||||||||||||
| // sources may call this to customize event emission, but it is | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Retry policy retries on all errors; add shouldRetry, jitter, and max cap
Blindly retrying on 4xx (e.g., 404 for rapidly-deleted records) adds latency with no benefit. Add a
shouldRetrypredicate, jitter, and amaxDelaycap.Apply:
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might as well have a 404 when we fetch "too early" and I think this was the case at least once in our testing. Hence the retry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mariczne That's a great point! If Airtable's API can return 404s for records that exist but aren't immediately available after webhook notifications (due to eventual consistency or propagation delays), then retrying on 404s makes perfect sense for this use case.
Your simpler retry implementation is appropriate here since the domain-specific behavior justifies retrying on all errors, including client errors like 404. Thanks for the clarification!
✏️ Learnings added