Skip to content

Commit

Permalink
fix: Fix default argument computation for objects (#510)
Browse files Browse the repository at this point in the history
<!--
Pull requests are squash merged using:
- their title as the commit message
- their description as the commit body

Having a good title and description is important for the users to get
readable changelog and understand when they need to update his code and
how.
-->

### Describe your change

Fix the default argument computation.
Make non-optional objects optional if all of its fields are optional.

### Motivation and context

[MET-295](https://metatype.atlassian.net/browse/MET-295)

### Migration notes

*N/A*

### Checklist

- [x] The change come with new or modified tests
- [x] Hard-to-understand functions have explanatory comments
- [x] ~End-user documentation is updated to reflect the change~: *N/A*
  • Loading branch information
Natoandro authored Dec 7, 2023
1 parent 67f04c4 commit 32394d9
Show file tree
Hide file tree
Showing 6 changed files with 201 additions and 16 deletions.
6 changes: 2 additions & 4 deletions meta-cli/src/typegraph/loader/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,12 +141,12 @@ impl Loader {
fn get_load_command(module_type: ModuleType, path: &Path) -> Result<Command, LoaderError> {
match module_type {
ModuleType::Python => {
// TODO cache result?
ensure_venv(path).map_err(|e| LoaderError::PythonVenvNotFound {
path: path.to_owned().into(),
error: e,
})?;
let vars: HashMap<_, _> = env::vars().collect();
// TODO cache result?
let mut command = Command::new("python3");
command
.arg(path.to_str().unwrap())
Expand Down Expand Up @@ -231,9 +231,7 @@ impl ToString for LoaderError {
format!("module file not found: {path:?}")
}
Self::PythonVenvNotFound { path, error } => {
format!(
"python venv (.venv) not found in parent directories of {path:?}: {error:?}"
)
format!("python venv (.venv) not found in parent directories of {path:?}: {error}",)
}
}
}
Expand Down
27 changes: 16 additions & 11 deletions typegate/src/engine/planner/args.ts
Original file line number Diff line number Diff line change
Expand Up @@ -571,13 +571,21 @@ class ArgumentCollector {
// no injection for the effect
// fallthrough
}
if (typ.type != Type.OPTIONAL) {
throw new MandatoryArgumentError(this.currentNodeDetails);
} else {
this.addPoliciesFrom(typ.item);

switch (typ.type) {
case Type.OPTIONAL: {
this.addPoliciesFrom(typ.item);
const { default_value: defaultValue = null } = typ;
return () => defaultValue;
}

case Type.OBJECT: {
return this.collectDefaults(typ.properties);
}

default:
throw new MandatoryArgumentError(this.currentNodeDetails);
}
const { default_value: defaultValue } = typ;
return () => defaultValue;
}

/** Collect the value of an injected parameter. */
Expand Down Expand Up @@ -673,11 +681,8 @@ class ArgumentCollector {
return typ.default_value;
}

const suggestions = `available fields from parent are: ${
Object.keys(
parent,
).join(", ")
}`;
const keys = Object.keys(parent).join(", ");
const suggestions = `available fields from parent are: ${keys}`;
throw new Error(
`non-optional injected argument ${name} is missing from parent: ${suggestions}`,
);
Expand Down
130 changes: 130 additions & 0 deletions typegate/tests/e2e/cli/deploy1_test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
// Copyright Metatype OÜ, licensed under the Elastic License 2.0.
// SPDX-License-Identifier: Elastic-2.0

import { gql, Meta } from "test-utils/mod.ts";
import { TestModule } from "test-utils/test_module.ts";
import { removeMigrations } from "test-utils/migrations.ts";
import { assertStringIncludes } from "std/assert/mod.ts";
import pg from "npm:pg";

const m = new TestModule(import.meta);

const tgName = "migration-failure-test";

/**
* These tests use different ports for the virtual typegate instance to avoid
* conflicts with one another when running in parallel.
*/

async function writeTypegraph(version: number | null) {
if (version == null) {
await m.shell([
"bash",
"-c",
"cp ./templates/migration.py migration.py",
]);
} else {
await m.shell([
"bash",
"select.sh",
"templates/migration.py",
`${version}`,
"migration.py",
]);
}
}

async function deploy(port: number | null, noMigration = false) {
const migrationOpts = noMigration ? [] : ["--create-migration"];

try {
const out = await m.cli(
{},
"deploy",
"--target",
port == null ? "dev" : `dev${port}`,
"-f",
"migration.py",
"--allow-dirty",
...migrationOpts,
"--allow-destructive",
);
if (out.stdout.length > 0) {
console.log(
`-- deploy STDOUT start --\n${out.stdout}-- deploy STDOUT end --`,
);
}
if (out.stderr.length > 0) {
console.log(
`-- deploy STDERR start --\n${out.stderr}-- deploy STDERR end --`,
);
}
} catch (e) {
console.log(e.toString());
throw e;
}
}

async function reset(schema: string) {
await removeMigrations(tgName);

// remove the database schema
const client = new pg.Client({
connectionString: "postgres://postgres:password@localhost:5432/db",
});
await client.connect();
await client.query(`DROP SCHEMA IF EXISTS ${schema} CASCADE`);
await client.end();
}

Meta.test(
"meta deploy: fails migration for new columns without default value",
async (t) => {
await t.should("load first version of the typegraph", async () => {
await reset("e2e7895alt");
await writeTypegraph(null);
});

const port = 7895;

// `deploy` must be run outside of the `should` block,
// otherwise this would fail by leaking ops.
// That is expected since it creates new engine that persists beyond the
// `should` block.
await deploy(port);

await t.should("insert records", async () => {
const e = t.getTypegraphEngine(tgName);
if (!e) {
throw new Error("typegraph not found");
}
await gql`
mutation {
createRecord(data: {}) {
id
}
}
`
.expectData({
createRecord: {
id: 1,
},
})
.on(e);
});

await t.should("load second version of the typegraph", async () => {
await writeTypegraph(1);
});

try {
await deploy(port);
} catch (e) {
assertStringIncludes(
e.message,
'column "age" of relation "Record" contains null values: set a default value:',
);
}
},
{ port: 7895, systemTypegraphs: true },
);
37 changes: 37 additions & 0 deletions typegate/tests/planner/default_args_test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Copyright Metatype OÜ, licensed under the Elastic License 2.0.
// SPDX-License-Identifier: Elastic-2.0

import { dropSchemas, recreateMigrations } from "test-utils/migrations.ts";
import { gql, Meta } from "test-utils/mod.ts";

Meta.test("prisma", async (t) => {
const e = await t.engine("runtimes/prisma/full_prisma_mapping.py", {
secrets: {
POSTGRES:
"postgresql://postgres:password@localhost:5432/db?schema=prisma",
},
});

await dropSchemas(e);
await recreateMigrations(e);

await t.should("fail", async () => {
await gql`
query CommentedAuthors {
findCommentersOfCurrentUser {
id
name
age
city
}
}
`
.withContext({
"user_id": 12,
})
.expectData({
findCommentersOfCurrentUser: [],
})
.on(e);
});
});
11 changes: 11 additions & 0 deletions typegate/tests/runtimes/prisma/full_prisma_mapping.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ def prisma(g: Graph):
"city": t.string(),
"posts": t.list(g.ref("Post")),
"extended_profile": g.ref("ExtendedProfile").optional(),
"comments": t.list(g.ref("Comment")),
},
name="User",
)
Expand All @@ -42,6 +43,7 @@ def prisma(g: Graph):
"id": t.integer(as_id=True),
"content": t.string(),
"related_post": g.ref("Post"),
"author": g.ref("User"),
},
name="Comment",
)
Expand All @@ -65,6 +67,15 @@ def prisma(g: Graph):
updateUser=db.update(user),
upsertOneUser=db.upsert(user),
findManyPosts=db.find_many(post),
findCommentersOfCurrentUser=db.find_many(user).reduce(
{
"where": {
"comments": {
"some": {"author": {"id": g.inherit().from_context("user_id")}}
}
}
}
),
findFirstPost=db.find_first(post),
findFirstComment=db.find_first(comment),
findUniquePost=db.find_unique(post),
Expand Down
6 changes: 5 additions & 1 deletion typegate/tests/runtimes/prisma/full_prisma_mapping_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,11 @@ Meta.test("prisma full mapping", async (t) => {
await gql`
mutation {
createOneComment(
data: { id: 50001, content: "good", related_post: {connect: {id: 10001}} }
data: {
id: 50001,
content: "good",
related_post: { connect: { id: 10001 }},
author: { connect: { id: 1 } } }
) {
id
content
Expand Down

0 comments on commit 32394d9

Please sign in to comment.