diff --git a/.changeset/fix-nested-fragment-argument-values.md b/.changeset/fix-nested-fragment-argument-values.md new file mode 100644 index 000000000..88201c479 --- /dev/null +++ b/.changeset/fix-nested-fragment-argument-values.md @@ -0,0 +1,6 @@ +--- +"houdini-core": patch +"houdini": patch +--- + +Fix nil panic when generating artifacts for fragments with nested @with directives, and make artifact generation deterministic by sorting keys consistently. diff --git a/packages/houdini-core/plugin/documents/artifacts/artifacts.go b/packages/houdini-core/plugin/documents/artifacts/artifacts.go index 3a3d26107..27e864871 100644 --- a/packages/houdini-core/plugin/documents/artifacts/artifacts.go +++ b/packages/houdini-core/plugin/documents/artifacts/artifacts.go @@ -105,9 +105,7 @@ func GenerateDocumentArtifacts( continue } - if fp != "" { - filepaths.Append(fp) - } + filepaths.Append(fp) } }() } diff --git a/packages/houdini-core/plugin/documents/artifacts/selection.go b/packages/houdini-core/plugin/documents/artifacts/selection.go index 6eb6388b6..fb8e14101 100644 --- a/packages/houdini-core/plugin/documents/artifacts/selection.go +++ b/packages/houdini-core/plugin/documents/artifacts/selection.go @@ -10,7 +10,6 @@ import ( "strings" "github.com/spf13/afero" - "code.houdinigraphql.com/packages/houdini-core/config" "code.houdinigraphql.com/packages/houdini-core/plugin/documents/artifacts/typescript" @@ -53,8 +52,9 @@ func writeSelectionDocument( artifactPath := projectConfig.ArtifactPath(name) // skip the write if the content hasn't changed (common on incremental runs) - if existing, err := afero.ReadFile(fs, artifactPath); err == nil && string(existing) == artifact { - return "", nil + if existing, err := afero.ReadFile(fs, artifactPath); err == nil && + string(existing) == artifact { + return artifactPath, nil } // write the file to disk @@ -1707,8 +1707,7 @@ func serializeFragmentArgument(arg *collected.ArgumentValue, level int) string { %s"name": { %s"kind": "Name", %s"value": "%s", -%s}, -%s"value": "%s"`, indent1, indent2, indent2, arg.Raw, indent1, indent1, arg.Raw) +%s}`, indent1, indent2, indent2, arg.Raw, indent1) case "String", "Enum": attrs = fmt.Sprintf(` %s"value": "%s"`, indent1, arg.Raw) diff --git a/packages/houdini-core/plugin/documents/artifacts/selection_conditional_test.go b/packages/houdini-core/plugin/documents/artifacts/selection_conditional_test.go index 4f53a73fd..4c709cb82 100644 --- a/packages/houdini-core/plugin/documents/artifacts/selection_conditional_test.go +++ b/packages/houdini-core/plugin/documents/artifacts/selection_conditional_test.go @@ -223,8 +223,7 @@ fragment UserDetails on User { "name": { "kind": "Name", "value": "show", - }, - "value": "show" + } } } }], @@ -360,8 +359,7 @@ fragment UserDetails on User { "name": { "kind": "Name", "value": "show", - }, - "value": "show" + } } } }], diff --git a/packages/houdini-core/plugin/documents/artifacts/selection_test.go b/packages/houdini-core/plugin/documents/artifacts/selection_test.go index ec88a9609..22970a904 100644 --- a/packages/houdini-core/plugin/documents/artifacts/selection_test.go +++ b/packages/houdini-core/plugin/documents/artifacts/selection_test.go @@ -2520,8 +2520,7 @@ fragment UserPetsByAge_qkFx4 on User { "name": { "kind": "Name", "value": "minAge", - }, - "value": "minAge" + } } }] }, diff --git a/packages/houdini-core/plugin/fragmentArguments/transform.go b/packages/houdini-core/plugin/fragmentArguments/transform.go index 7759a0b07..fade67c7c 100644 --- a/packages/houdini-core/plugin/fragmentArguments/transform.go +++ b/packages/houdini-core/plugin/fragmentArguments/transform.go @@ -8,8 +8,6 @@ import ( "sync" "golang.org/x/sync/syncmap" - - "code.houdinigraphql.com/plugins" "code.houdinigraphql.com/plugins/graphql" @@ -57,6 +55,7 @@ func Transform[PluginConfig any](ctx context.Context, db plugins.DatabasePool[Pl AND selection_directives.directive = 'with' AND (raw_documents.current_task = $task_id OR $task_id IS NULL) AND (documents.processed = false OR documents.processed IS NULL) + AND (documents.internal = false OR documents.internal IS NULL) GROUP BY documents.id `) if err != nil { @@ -253,7 +252,8 @@ func processDocument[PluginConfig any]( parent_doc.name as document, selection_refs.id as selection_ref, selections.id as selection_id, - selections.field_name as fragment, + selections.field_name as current_name, + COALESCE(selections.fragment_ref, selections.field_name) as fragment, fragment_doc.id as fragment_doc_id, json_group_array( json_object( @@ -278,15 +278,14 @@ func processDocument[PluginConfig any]( ) END as doc_variables, fragment_doc.type_condition as type_condition, - fragment_doc.raw_document as raw_document, - selections.fragment_ref as fragment_ref + fragment_doc.raw_document as raw_document FROM selection_directives JOIN selections ON selection_directives.selection_id = selections.id JOIN selection_refs ON selection_refs.child_id = selections.id JOIN selection_directive_arguments ON selection_directives.id = selection_directive_arguments.parent AND selection_directive_arguments.document = $document JOIN argument_values as selection_arg_values ON selection_directive_arguments."value" = selection_arg_values.id AND selection_arg_values.document = $document JOIN documents as parent_doc ON selection_refs."document" = parent_doc.id - JOIN documents as fragment_doc on selections.field_name = fragment_doc.name + JOIN documents as fragment_doc on COALESCE(selections.fragment_ref, selections.field_name) = fragment_doc.name LEFT JOIN document_variables on fragment_doc.id = document_variables."document" LEFT JOIN argument_values as document_variable_default_values on document_variable_default_values.id = document_variables.default_value WHERE selection_directives.directive = $with_directive @@ -307,11 +306,11 @@ func processDocument[PluginConfig any]( selectionID := withSearch.GetInt64("selection_id") fragmentDocID := withSearch.GetInt64("fragment_doc_id") fragmentName := withSearch.GetText("fragment") + currentFieldName := withSearch.GetText("current_name") withArgsStr := withSearch.GetText("with_args") docVariablesStr := withSearch.GetText("doc_variables") typeCondition := withSearch.GetText("type_condition") rawDocument := withSearch.GetInt64("raw_document") - fragmentRef := withSearch.GetText("fragment_ref") withArgs := []struct { Name string `json:"name"` @@ -389,9 +388,8 @@ func processDocument[PluginConfig any]( hash := murmurHash(string(args)) newFragmentName := fragmentName + "_" + hash - // if the selection has already been transformed, don't transfor it again - expectedName := fragmentRef + "_" + hash - if fragmentName == expectedName { + // if the selection's field_name already matches the expected clone for this scope, skip + if currentFieldName == newFragmentName { return } @@ -731,12 +729,22 @@ func cloneDocument[PluginConfig any]( name := statements.NoSelectionArgsDirectiveArgsSearch.GetText("name") value := statements.NoSelectionArgsDirectiveArgsSearch.GetInt64("value") - // insert a directive document for the new document with the mapped value + // create a fresh copy of the argument value exclusively for this directive arg so + // it doesn't share a row with field/nested arg uses of the same variable + err = db.ExecStatement(statements.CopyArgumentValue, map[string]any{ + "id": valueMap[value], + "document": documentID, + }) + if err != nil { + return + } + freshCopy := conn.LastInsertRowID() + err = db.ExecStatement(statements.InsertSelectionDirectiveArgument, map[string]any{ "name": name, "parent": parent, - "value": valueMap[value], + "value": freshCopy, "document": documentID, }) }, @@ -874,13 +882,24 @@ func cloneDocument[PluginConfig any]( directiveID := conn.LastInsertRowID() for _, arg := range directive.Arguments { - // add the corresponding directive argument + // create a fresh copy exclusively for this directive arg so it doesn't + // share an argument_values row with field/nested arg uses of the same variable + err = db.ExecStatement(statements.CopyArgumentValue, map[string]any{ + "id": valueMap[arg.Value], + "document": documentID, + }) + if err != nil { + errs.Append(plugins.WrapError(err)) + return + } + freshCopy := conn.LastInsertRowID() + err = db.ExecStatement( statements.InsertSelectionDirectiveArgument, map[string]any{ "parent": directiveID, "name": arg.Name, - "value": valueMap[arg.Value], + "value": freshCopy, "document": documentID, }, ) @@ -925,7 +944,13 @@ func cloneDocument[PluginConfig any]( } // before we finish lets figure out the new scope - newScope, descendantVarNames, err := statements.CopyScope(ctx, db, conn, fragmentScope, documentID) + newScope, descendantVarNames, err := statements.CopyScope( + ctx, + db, + conn, + fragmentScope, + documentID, + ) if err != nil { return 0, nil, nil, err } @@ -1708,12 +1733,21 @@ func (s *transformStatements[PluginConfig]) ReplaceVariables( ) error { errs := &plugins.ErrorList{} + // track values we've already written as replacements so SQLite cursor re-scans don't + // pick them up and incorrectly nullify them + alreadyReplaced := map[int64]bool{} + db.BindStatement(search, map[string]any{"document": documentID}) err := db.StepStatement(ctx, search, func() { parentValue := search.GetInt64("parent") variableName := search.GetText("variable") oldValue := search.GetInt64("value") + // skip values we already placed as replacements in this pass + if alreadyReplaced[oldValue] { + return + } + // if the variable name is not defined in the scope then we need to delete the original // value and replace it with null otherwise we'll replace it with the scope value scopeValue, ok := scope[variableName] @@ -1748,6 +1782,9 @@ func (s *transformStatements[PluginConfig]) ReplaceVariables( return } + // mark the replacement so cursor re-scans don't nullify it + alreadyReplaced[scopeValue] = true + // by now, the value passed to the scope will replace the old value so we need to delete it err = db.ExecStatement(s.DeleteValue, map[string]any{"id": oldValue}) if err != nil { diff --git a/packages/houdini-core/plugin/fragmentArguments/transform_test.go b/packages/houdini-core/plugin/fragmentArguments/transform_test.go index 151cbb9ea..fa5eebf55 100644 --- a/packages/houdini-core/plugin/fragmentArguments/transform_test.go +++ b/packages/houdini-core/plugin/fragmentArguments/transform_test.go @@ -22,10 +22,14 @@ func TestFragmentArgumentTransform(t *testing.T) { type User { firstName: String! - friends(name: String, limit: Int, offset: Int): [User!]! + friends(name: String, limit: Int, offset: Int, filter: FriendFilter): [User!]! friendsByNames(names: [String!]): [User!]! id: ID! } + + input FriendFilter { + name: String! + } `, Tests: []tests.Test[config.PluginConfig]{ { @@ -73,10 +77,10 @@ func TestFragmentArgumentTransform(t *testing.T) { Type: "String", TypeModifiers: "!", }), + }, }, - }, - { - Name: "Passes argument values to generated fragments", + { + Name: "Passes argument values to generated fragments", Pass: true, Input: []string{ ` @@ -505,10 +509,19 @@ func TestFragmentArgumentTransform(t *testing.T) { ), tests.ExpectedDoc( `fragment F_3fkJCt on User { friendsA: friends(name: $shared) { firstName id __typename } friendsB: friends(name: $renamed) { firstName id __typename } id __typename }`, - ).WithVariables( - tests.ExpectedOperationVariable{Name: "a", Type: "String", TypeModifiers: "!"}, - tests.ExpectedOperationVariable{Name: "b", Type: "String", TypeModifiers: "!"}, - ), + ). + WithVariables( + tests.ExpectedOperationVariable{ + Name: "a", + Type: "String", + TypeModifiers: "!", + }, + tests.ExpectedOperationVariable{ + Name: "b", + Type: "String", + TypeModifiers: "!", + }, + ), }, }, { @@ -550,6 +563,22 @@ func TestFragmentArgumentTransform(t *testing.T) { ).WithVariables(tests.ExpectedOperationVariable{Name: "name", Type: "String", TypeModifiers: "!"}), }, }, + { + Name: "Argument used in object", + Pass: true, + Input: []string{ + `query Q($name: String!) { user { ...F @with(name: $name) } }`, + `fragment F on User @arguments(name: {type: "String!"}) { friends(filter: {name: $name}) { firstName } }`, + }, + Expected: []tests.ExpectedDocument{ + tests.ExpectedDoc( + `query Q($name: String!) { user { ...F_4E9dx0 @with(name: $name) id __typename } }`, + ), + tests.ExpectedDoc( + `fragment F_4E9dx0 on User { id __typename friends(filter: {name: $name}) {firstName id __typename }}`, + ).WithVariables(tests.ExpectedOperationVariable{Name: "name", Type: "String", TypeModifiers: "!"}), + }, + }, { Name: "Fragment spreads fragment with renamed variable argument", Pass: true, @@ -570,6 +599,66 @@ func TestFragmentArgumentTransform(t *testing.T) { ).WithVariables(tests.ExpectedOperationVariable{Name: "innerName", Type: "String", TypeModifiers: "!"}), }, }, + { + Name: "Renamed variable used in both field arg and nested @with", + Pass: true, + Input: []string{ + `query Q($studentId: String!) { user { ...Outer @with(name: $studentId) } }`, + `fragment Outer on User @arguments(name: {type: "String!"}) { friends(name: $name) { firstName } ...Inner @with(innerName: $name) }`, + `fragment Inner on User @arguments(innerName: {type: "String!"}) { friends(name: $innerName) { firstName } }`, + }, + Expected: []tests.ExpectedDocument{ + tests.ExpectedDoc( + `query Q($studentId: String!) { user { ...Outer_3TkJtv @with(name: $studentId) id __typename } }`, + ), + tests.ExpectedDoc( + `fragment Outer_3TkJtv on User { id __typename friends(name: $studentId) { firstName id __typename } ...Inner_2Lx8Sp @with(innerName: $studentId) }`, + ).WithVariables(tests.ExpectedOperationVariable{Name: "name", Type: "String", TypeModifiers: "!"}), + tests.ExpectedDoc( + `fragment Inner_2Lx8Sp on User { friends(name: $studentId) { firstName id __typename } id __typename }`, + ).WithVariables(tests.ExpectedOperationVariable{Name: "innerName", Type: "String", TypeModifiers: "!"}), + }, + }, + { + Name: "Renamed variable used in nested field object arg and nested @with", + Pass: true, + Input: []string{ + `query Q($studentId: String!) { user { ...Outer @with(name: $studentId) } }`, + `fragment Outer on User @arguments(name: {type: "String!"}) { friends(filter: {name: $name}) { firstName } ...Inner @with(innerName: $name) }`, + `fragment Inner on User @arguments(innerName: {type: "String!"}) { friends(name: $innerName) { firstName } }`, + }, + Expected: []tests.ExpectedDocument{ + tests.ExpectedDoc( + `query Q($studentId: String!) { user { ...Outer_3TkJtv @with(name: $studentId) id __typename } }`, + ), + tests.ExpectedDoc( + `fragment Outer_3TkJtv on User { id __typename friends(filter: {name: $studentId}) { firstName id __typename } ...Inner_2Lx8Sp @with(innerName: $studentId) }`, + ).WithVariables(tests.ExpectedOperationVariable{Name: "name", Type: "String", TypeModifiers: "!"}), + tests.ExpectedDoc( + `fragment Inner_2Lx8Sp on User { friends(name: $studentId) { firstName id __typename } id __typename }`, + ).WithVariables(tests.ExpectedOperationVariable{Name: "innerName", Type: "String", TypeModifiers: "!"}), + }, + }, + { + Name: "Renamed variable used only in nested @with (no field args)", + Pass: true, + Input: []string{ + `query Q($studentId: String!) { user { ...Outer @with(name: $studentId) } }`, + `fragment Outer on User @arguments(name: {type: "String!"}) { ...Inner @with(innerName: $name) }`, + `fragment Inner on User @arguments(innerName: {type: "String!"}) { friends(name: $innerName) { firstName } }`, + }, + Expected: []tests.ExpectedDocument{ + tests.ExpectedDoc( + `query Q($studentId: String!) { user { ...Outer_3TkJtv @with(name: $studentId) id __typename } }`, + ), + tests.ExpectedDoc( + `fragment Outer_3TkJtv on User { id __typename ...Inner_2Lx8Sp @with(innerName: $studentId) }`, + ).WithVariables(tests.ExpectedOperationVariable{Name: "name", Type: "String", TypeModifiers: "!"}), + tests.ExpectedDoc( + `fragment Inner_2Lx8Sp on User { friends(name: $studentId) { firstName id __typename } id __typename }`, + ).WithVariables(tests.ExpectedOperationVariable{Name: "innerName", Type: "String", TypeModifiers: "!"}), + }, + }, }, }) } @@ -658,8 +747,8 @@ func TestFragmentArgumentTransform_multipleRuns(t *testing.T) { TypeModifiers: "!", }, ), + }, }, }, - }, }) } diff --git a/packages/houdini/src/lib/codegen.ts b/packages/houdini/src/lib/codegen.ts index 83f1a4054..f5884e354 100644 --- a/packages/houdini/src/lib/codegen.ts +++ b/packages/houdini/src/lib/codegen.ts @@ -443,11 +443,7 @@ export async function codegen_setup( logger.time(`Spawn ${plugin.name}`) const child = spawn(executable, args, { - // [stdin, stdout, stderr]: stdio plugins need piped stdin/stdout for the - // message protocol; stderr is always inherited so plugin logs reach the terminal. - stdio: pluginUsesStdio - ? ['pipe', 'pipe', 'inherit'] - : ['inherit', 'inherit', 'inherit'], + stdio: ['pipe', 'pipe', 'inherit'], detached: process.platform !== 'win32', }) diff --git a/packages/houdini/src/runtime/cache/index.ts b/packages/houdini/src/runtime/cache/index.ts index 9dfea25c5..9dfa8ca81 100644 --- a/packages/houdini/src/runtime/cache/index.ts +++ b/packages/houdini/src/runtime/cache/index.ts @@ -951,9 +951,13 @@ class CacheInternal { if (operation.parentID) { // if its a normal scalar we can use the value directly if (operation.parentID.kind !== 'Variable') { - parentID = operation.parentID.value + parentID = + 'value' in operation.parentID + ? String(operation.parentID.value) + : undefined } else { - const id = variables[operation.parentID.value] + const varName = operation.parentID.name.value + const id = variables[varName] if (typeof id !== 'string') { throw new Error('parentID value must be a string') } @@ -966,9 +970,11 @@ class CacheInternal { let opaqueListID: string | undefined if (operation.listID) { if (operation.listID.kind !== 'Variable') { - opaqueListID = operation.listID.value + opaqueListID = + 'value' in operation.listID ? String(operation.listID.value) : undefined } else { - const id = variables[operation.listID.value] + const varName = operation.listID.name.value + const id = variables[varName] if (typeof id !== 'string') { throw new Error('listID value must be a string') } diff --git a/packages/houdini/src/runtime/types.ts b/packages/houdini/src/runtime/types.ts index 3c2254884..0a91a6538 100644 --- a/packages/houdini/src/runtime/types.ts +++ b/packages/houdini/src/runtime/types.ts @@ -179,14 +179,8 @@ export type MutationOperation = { action: 'insert' | 'remove' | 'delete' | 'toggle' | 'upsert' list?: string type?: string - parentID?: { - kind: string - value: string - } - listID?: { - kind: string - value: string - } + parentID?: ValueNode + listID?: ValueNode position?: 'first' | 'last' target?: 'all' // when conditions are encoded as filter nodes so that variable references