Configure hasura in single bulk request - attempt to fix a race condition - #770
Conversation
WalkthroughIntroduces a bulk Hasura metadata workflow using a keep-going endpoint. Adds a bulkOperation type, builders for select permissions and relationships, and a single executeBulkKeepGoing call to run collected operations. Updates logging/messages and implements structured parsing and reporting of partial and execution errors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as Envió CLI
participant Hasura as Hasura Metadata API
rect rgb(235, 245, 255)
note over CLI: Build operations
CLI->>CLI: collect select-permission ops
CLI->>CLI: collect relationship ops
end
rect rgb(240, 255, 240)
note over CLI,Hasura: Bulk execution (keep-going)
CLI->>Hasura: POST /metadata (bulkKeepGoing, operations[])
Hasura-->>CLI: results[] (successes + errors)
end
alt Any operation errored
CLI->>CLI: parse results, map errors
CLI-->>CLI: warn on partial failures
else All succeeded
CLI-->>CLI: log completion
end
opt Transport/exec error
CLI-->>CLI: log error with exception details
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
codegenerator/cli/npm/envio/src/Hasura.res (8)
91-104: Preserve error context (code/path) when parsing bulk results.You currently keep only the error string. Including code/path will make debugging far easier.
- let bulkKeepGoingErrorsSchema = S.array( - S.union([ - S.object(s => { - s.tag("message", "success") - None - }), - S.object(s => { - Some(s.field("error", S.string)) - }), - ]), - )->S.transform(_ => { - parser: a => Belt.Array.keepMapU(a, a => a), - }) +let bulkKeepGoingErrorsSchema = S.array( + S.union([ + S.object(s => { + s.tag("message", "success") + None + }), + S.object(s => { + let error = s.field("error", S.string) + let code = s.field("code", S.string)->S.optional + let path = s.field("path", S.string)->S.optional + Some({"error": error, "code": code, "path": path}) + }), + ]), +)->S.transform(_ => { + // keep only failures, but retain rich context + parser: a => Belt.Array.keepMapU(a, a => a), +})
164-168: Constrain operation.type to a variant to avoid stringly-typed mistakes.Using a polymorphic variant for known Hasura metadata ops prevents typos and enables exhaustiveness checks.
-type bulkOperation = { - "type": string, - args: Js.Json.t, -} +type bulkOpType = [#pg_create_select_permission | #pg_create_array_relationship | #pg_create_object_relationship] +type bulkOperation = { + "type": bulkOpType, + args: Js.Json.t, +}
227-269: Improve failure logs by tying errors back to operations.Today we only log a flat list of errors. If you also parse the full result array, you can zip results with operations to include the failing op’s type/name and index for faster triage.
294-305: Precompute aggregate set once; pass boolean to builder.Minor perf/clarity: compute a Belt.Set.String from aggregateEntities and pass allowAggregations as a boolean.
- // Collect all operations for bulk execution -let allOperations = [] +// Collect all operations for bulk execution +let allOperations = [] +let aggregateSet = Belt.Set.String.fromArray(aggregateEntities) ... - createSelectPermissionOperation(~tableName, ~pgSchema, ~responseLimit, ~aggregateEntities), + createSelectPermissionOperation( + ~tableName, + ~pgSchema, + ~responseLimit, + ~allowAggregations=aggregateSet->Belt.Set.String.has(tableName), + ),
306-351: Avoid unwrapResultExn; skip bad mappings gracefully.unwrapResultExn will crash the whole bulk build if a mapping is missing. Prefer handling the error and skipping that single relationship with a warning.
- let relationalFieldName = - schema->Schema.getDerivedFromFieldName(derivedFromField)->Utils.unwrapResultExn + let relationalFieldNameResult = schema->Schema.getDerivedFromFieldName(derivedFromField) + switch relationalFieldNameResult { + | Error(err) => + Logging.warn({ + "msg": "Skipping array relationship; failed to resolve derivedFrom mapping", + "tableName": tableName, + "field": derivedFromField.fieldName, + "err": err, + }) + // skip this item + | Ok(relationalFieldName) => + // push operation as before using relationalFieldName + }
353-354: Final bulk execution step — nice consolidation.This addresses the race by batching. Consider adding a short summary log of how many operations were successes vs failures.
169-192: Minor: speed upallow_aggregationslookup by precomputing a SetRather than calling
aggregateEntities->Js.Array2.includes(tableName)(O(n)) in each invocation, change the builder’s signature and call sites as follows:- let createSelectPermissionOperation = (~tableName, ~pgSchema, ~responseLimit, ~aggregateEntities): bulkOperation => { + let createSelectPermissionOperation = (~tableName, ~pgSchema, ~responseLimit, ~allowAggregations: bool): bulkOperation => { { "type": "pg_create_select_permission", args: { "table": { "schema": pgSchema, "name": tableName }, "role": "public", "source": "default", "permission": { "columns": "*", "filter": Js.Obj.empty(), "limit": responseLimit, - "allow_aggregations": aggregateEntities->Js.Array2.includes(tableName), + "allow_aggregations": allowAggregations, }, }->(Utils.magic: 'a => Js.Json.t), } }At the call site, define the set once and pass the boolean:
+ let aggregateSet = Belt.Set.String.fromArray(aggregateEntities) - createSelectPermissionOperation(~tableName, ~pgSchema, ~responseLimit, ~aggregateEntities) + createSelectPermissionOperation( + ~tableName, + ~pgSchema, + ~responseLimit, + ~allowAggregations=aggregateSet->Belt.Set.String.has(tableName), +)“columns”: “*” remains valid in the Hasura metadata API—no change needed.
74-90: Bulk route contract verifiedPer Hasura v2.24.0, bulk_keep_going always returns HTTP 200 with per-item results: successes as
{"message":"success"}and errors inline as objects containing at leasterror,path, and optionalcodefields. No non-200 responses are expected, but add explicit non-200 mappings in your Rest layer to handle any unmatched status codes and avoid “no matching response” errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codegenerator/cli/npm/envio/src/Hasura.res(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{res,resi}
📄 CodeRabbit inference engine (.cursor/rules/rescript.mdc)
**/*.{res,resi}: Never use[| item |]to create an array. Use[ item ]instead.
Must always use=for setting value to a field. Use:=only for ref values created usingreffunction.
ReScript has record types which require a type definition before hand. You can access record fields by dot likefoo.myField.
It's also possible to define an inline object, it'll have quoted fields in this case.
Use records when working with structured data, and objects to conveniently pass payload data between functions.
Never use %raw to access object fields if you know the type.
Files:
codegenerator/cli/npm/envio/src/Hasura.res
**/*.res
📄 CodeRabbit inference engine (.cursor/rules/navigation.mdc)
Prefer reading ReScript .res modules directly; compiled .js artifacts can be ignored
Files:
codegenerator/cli/npm/envio/src/Hasura.res
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (2)
codegenerator/cli/npm/envio/src/Hasura.res (2)
109-111: Copy reads better; OK.
147-149: Copy reads better; OK.
| let createEntityRelationshipOperation = ( | ||
| ~pgSchema, | ||
| ~endpoint, | ||
| ~auth, | ||
| ~tableName: string, | ||
| ~relationshipType: string, | ||
| ~relationalKey: string, | ||
| ~objectName: string, | ||
| ~mappedEntity: string, | ||
| ~isDerivedFrom: bool, | ||
| ) => { | ||
| ): bulkOperation => { | ||
| let derivedFromTo = isDerivedFrom ? `"id": "${relationalKey}"` : `"${relationalKey}_id" : "id"` | ||
|
|
||
| let bodyString = `{"type": "pg_create_${relationshipType}_relationship","args": {"table": {"schema": "${pgSchema}", "name": "${tableName}"},"name": "${objectName}","source": "default","using": {"manual_configuration": {"remote_table": {"schema": "${pgSchema}", "name": "${mappedEntity}"},"column_mapping": {${derivedFromTo}}}}}}` | ||
|
|
||
| try { | ||
| let result = await rawBodyRoute->Rest.fetch( | ||
| { | ||
| "auth": auth, | ||
| "bodyString": bodyString, | ||
| { | ||
| \"type": `pg_create_${relationshipType}_relationship`, | ||
| args: { | ||
| "table": { | ||
| "schema": pgSchema, | ||
| "name": tableName, | ||
| }, | ||
| ~client=Rest.client(endpoint), | ||
| ) | ||
| let msg = switch result { | ||
| | QuerySucceeded => `Hasura ${relationshipType} relationship created` | ||
| | AlreadyDone => `Hasura ${relationshipType} relationship already created` | ||
| "name": objectName, | ||
| "source": "default", | ||
| "using": { | ||
| "manual_configuration": { | ||
| "remote_table": { | ||
| "schema": pgSchema, | ||
| "name": mappedEntity, | ||
| }, | ||
| "column_mapping": Js.Json.parseExn(`{${derivedFromTo}}`), | ||
| }, | ||
| }, | ||
| }->(Utils.magic: 'a => Js.Json.t), | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Avoid Json.parseExn; build column_mapping structurally to prevent runtime parse errors.
Parsing JSON strings for column_mapping is brittle and can throw if names ever include unexpected chars. Build a dict/object instead.
-let createEntityRelationshipOperation = (
+let createEntityRelationshipOperation = (
~pgSchema,
~tableName: string,
- ~relationshipType: string,
+ ~relationshipType: string,
~relationalKey: string,
~objectName: string,
~mappedEntity: string,
~isDerivedFrom: bool,
): bulkOperation => {
- let derivedFromTo = isDerivedFrom ? `"id": "${relationalKey}"` : `"${relationalKey}_id" : "id"`
-
- {
- "type": `pg_create_${relationshipType}_relationship`,
+ // Validate relationshipType and compute the Hasura op type
+ let type_ = switch relationshipType {
+ | "array" => "pg_create_array_relationship"
+ | "object" => "pg_create_object_relationship"
+ | other => Js.Exn.raiseError("Invalid relationshipType: " ++ other)
+ }
+ // Build column_mapping safely
+ let columnMapping: Js.Dict.t<string> = {
+ let m = Js.Dict.empty()
+ if isDerivedFrom {
+ Js.Dict.set(m, "id", relationalKey)
+ } else {
+ Js.Dict.set(m, Belt.String.concat(relationalKey, "_id"), "id")
+ }
+ m
+ }
+ {
+ "type": type_,
args: {
"table": {
"schema": pgSchema,
"name": tableName,
},
"name": objectName,
"source": "default",
"using": {
"manual_configuration": {
"remote_table": {
"schema": pgSchema,
"name": mappedEntity,
},
- "column_mapping": Js.Json.parseExn(`{${derivedFromTo}}`),
+ "column_mapping": columnMapping,
},
},
}->(Utils.magic: 'a => Js.Json.t),
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let createEntityRelationshipOperation = ( | |
| ~pgSchema, | |
| ~endpoint, | |
| ~auth, | |
| ~tableName: string, | |
| ~relationshipType: string, | |
| ~relationalKey: string, | |
| ~objectName: string, | |
| ~mappedEntity: string, | |
| ~isDerivedFrom: bool, | |
| ) => { | |
| ): bulkOperation => { | |
| let derivedFromTo = isDerivedFrom ? `"id": "${relationalKey}"` : `"${relationalKey}_id" : "id"` | |
| let bodyString = `{"type": "pg_create_${relationshipType}_relationship","args": {"table": {"schema": "${pgSchema}", "name": "${tableName}"},"name": "${objectName}","source": "default","using": {"manual_configuration": {"remote_table": {"schema": "${pgSchema}", "name": "${mappedEntity}"},"column_mapping": {${derivedFromTo}}}}}}` | |
| try { | |
| let result = await rawBodyRoute->Rest.fetch( | |
| { | |
| "auth": auth, | |
| "bodyString": bodyString, | |
| { | |
| \"type": `pg_create_${relationshipType}_relationship`, | |
| args: { | |
| "table": { | |
| "schema": pgSchema, | |
| "name": tableName, | |
| }, | |
| ~client=Rest.client(endpoint), | |
| ) | |
| let msg = switch result { | |
| | QuerySucceeded => `Hasura ${relationshipType} relationship created` | |
| | AlreadyDone => `Hasura ${relationshipType} relationship already created` | |
| "name": objectName, | |
| "source": "default", | |
| "using": { | |
| "manual_configuration": { | |
| "remote_table": { | |
| "schema": pgSchema, | |
| "name": mappedEntity, | |
| }, | |
| "column_mapping": Js.Json.parseExn(`{${derivedFromTo}}`), | |
| }, | |
| }, | |
| }->(Utils.magic: 'a => Js.Json.t), | |
| } | |
| } | |
| let createEntityRelationshipOperation = ( | |
| ~pgSchema, | |
| ~tableName: string, | |
| ~relationshipType: string, | |
| ~relationalKey: string, | |
| ~objectName: string, | |
| ~mappedEntity: string, | |
| ~isDerivedFrom: bool, | |
| ): bulkOperation => { | |
| // Validate relationshipType and compute the Hasura op type | |
| let type_ = switch relationshipType { | |
| | "array" => "pg_create_array_relationship" | |
| | "object" => "pg_create_object_relationship" | |
| | other => Js.Exn.raiseError("Invalid relationshipType: " ++ other) | |
| } | |
| // Build column_mapping safely | |
| let columnMapping: Js.Dict.t<string> = { | |
| let m = Js.Dict.empty() | |
| if isDerivedFrom { | |
| Js.Dict.set(m, "id", relationalKey) | |
| } else { | |
| Js.Dict.set(m, Belt.String.concat(relationalKey, "_id"), "id") | |
| } | |
| m | |
| } | |
| { | |
| "type": type_, | |
| args: { | |
| "table": { | |
| "schema": pgSchema, | |
| "name": tableName, | |
| }, | |
| "name": objectName, | |
| "source": "default", | |
| "using": { | |
| "manual_configuration": { | |
| "remote_table": { | |
| "schema": pgSchema, | |
| "name": mappedEntity, | |
| }, | |
| "column_mapping": columnMapping, | |
| }, | |
| }, | |
| }->(Utils.magic: 'a => Js.Json.t), | |
| } | |
| } |
🤖 Prompt for AI Agents
In codegenerator/cli/npm/envio/src/Hasura.res around lines 194 to 226, avoid
using Js.Json.parseExn for column_mapping; instead build the mapping as a
ReScript/Reason structure and convert it to Js.Json directly to prevent runtime
parse errors. Create a mapping (e.g. a Js.Dict or Belt.Map) whose single entry
is either ("id" -> relationalKey) when isDerivedFrom is true or (relationalKey ^
"_id" -> "id") otherwise, then convert that mapping to a Js.Json object and pass
it as "column_mapping" rather than parsing a JSON string; ensure the final value
is the expected Js.Json.t type used by Utils.magic cast.
JasoonS
left a comment
JasoonS
left a comment
There was a problem hiding this comment.
Nice that you could build this 👍 Looks good to me
2 participants
Summary by CodeRabbit