feat(qe): Implement findFirstOrThrow and findUniqeOrThrow in the engine #3356

miguelff · 2022-10-31T11:28:58Z

This PR implements the following APIs in the engine:

findFirstOrThrowModelName
findUniqueOrThrowModelName

Both APIs are currently implemented in the client side using findFirstModelName and findUniqueModelName engine APIs, and throwing an error in case they return 0 results.

After this change is merged, the client would need to remove the code that handles findUniqueOrThrow and findFirstOrThrow to use instead the newly created query-engine APIs.

I will work on a PR in prisma/prisma to address client changes next.

query-engine/core/src/interpreter/query_interpreters/read.rs

miguelff · 2022-11-02T15:13:34Z

query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/simple/find_first.rs

@@ -35,6 +35,19 @@ mod find_first_query {
        Ok(())
    }

+    #[connector_test]


Added a failure case for findFirst to check that it actually behaves differently than findFirstOrThrow

miguelff · 2022-11-02T15:14:08Z

query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/simple/find_first.rs

@@ -5,7 +5,7 @@ mod find_first_query {
    use query_engine_tests::assert_query;

    #[connector_test]
-    async fn fetch_first_matching(runner: Runner) -> TestResult<()> {


Did a small replacement of fetch -> find on the test cases

miguelff · 2022-11-02T15:14:45Z

...-engine/connector-test-kit-rs/query-engine-tests/tests/queries/simple/find_first_or_throw.rs

+    use query_engine_tests::assert_query;
+
+    #[connector_test]
+    async fn find_first_or_throw_matching(runner: Runner) -> TestResult<()> {


Test case to ensure that findFirstOrThrow behaves like findFirst when a record is found

miguelff · 2022-11-02T15:17:20Z

...engine/connector-test-kit-rs/query-engine-tests/tests/queries/simple/find_unique_or_throw.rs

+            2025,
+            "An operation failed because it depends on one or more records that were required but not found. Expected a record, found none."


This is the error returned to the user, which as per this method is a ConnectorError rendering this

miguelff · 2022-11-02T15:21:17Z

query-engine/core/src/query_graph_builder/builder.rs

@@ -63,7 +63,9 @@ impl QueryGraphBuilder {

        let mut graph = match (&query_info.tag, query_info.model.clone()) {
            (QueryTag::FindUnique, Some(m)) => read::find_unique(parsed_field, m).map(Into::into),
+            (QueryTag::FindUniqueOrThrow, Some(m)) => read::find_unique_or_throw(parsed_field, m).map(Into::into),


In order to build the new APIs I reused first/many/one modules in query_builder. I added a field of type QueryOptionsto RecordQuery and ManyRecordsQuery. See query_ast/read.rs

query-engine/core/src/query_ast/read.rs

miguelff · 2022-11-02T15:30:45Z

query-engine/core/src/query_graph_builder/read/many.rs

+#[inline]
+fn find_many_with_options(
+    field: ParsedField,
+    model: ModelRef,
+    options: QueryOptions,
+) -> QueryGraphBuilderResult<ReadQuery> {


This is the old implementation, which now receives QueryOptions. Previous find_many keeps the signature and passes no options QueryOptions::none, while find_many_or_throw builds a query with the THROW_ON_EMPTY option

Weakky

I left a few comments, none of them require you to change anything. They're open suggestions/nitpicks. Awesome work 💯!

query-engine/schema-builder/src/output_types/query_type.rs

miguelff · 2022-11-04T10:04:10Z

query-engine/core/src/query_ast/read.rs

@@ -96,6 +104,7 @@ pub struct RecordQuery {
    pub nested: Vec<ReadQuery>,
    pub selection_order: Vec<String>,
    pub aggregation_selections: Vec<RelAggregationSelection>,
+    pub options: BitFlags<QueryOption>,


It's more elegant and better now, @Weakky . Thank you so much for the suggestion!

garrensmith

This is looking good. I've left some comments around the QueryOptions. lets try and make that abstraction cleaner. Great work.

query-engine/core/src/interpreter/query_interpreters/read.rs

garrensmith · 2022-11-04T10:59:41Z

query-engine/core/src/query_graph/mod.rs

@@ -818,6 +819,7 @@ impl QueryGraph {
                nested: vec![],
                selection_order: vec![],
                aggregation_selections: vec![],
+                options: BitFlags::EMPTY,


I think rather doing options: QueryOptions::none would be better here. That would require adding a new query option but then we keep with the QueryOption abstraction rather than exposing the underlying BitFlags.

Or you could also do options: Default::default and implement the default interface.

garrensmith · 2022-11-04T11:03:26Z

query-engine/core/src/query_graph_builder/read/many.rs

+}
+
+pub fn find_many_or_throw(field: ParsedField, model: ModelRef) -> QueryGraphBuilderResult<ReadQuery> {
+    find_many_with_options(field, model, BitFlags::from_flag(QueryOption::ThrowOnEmpty))


Same here, we leaking the BitFlag abstraction here. Is there another way around this. We want to use BitFlags internally but not expose that outside the QueryOption.

you might have to have something like:

struct QueryOption { inner: BitFlags }

miguelff · 2022-11-04T13:07:12Z

Hey, @garrensmith Thank you so much for your feedback!

Although I see how generally "leaky abstraction" constitutes an antipattern, the case of BitFlags is IMO slightly different. BitFlags is a generic collection of items packed in a bit array with set semantics. Similar to other collections in the standard library which are used in many places, they don't constitute a leaky abstraction (or at least not to the same extent as it would be to leak a data structure that's specific to our domain model)

Having said that I reckon it would be better not to depend on importing the enum flags library each time we want to use any options. It's too involved for what it does! 👍🏾

I would try to hide some implementation details in the next commit and request your review again. Thank you! ✨

…ible

miguelff · 2022-11-04T14:04:25Z

@garrensmith Some feedback was addressed in 0dd9c8a . Ready to be merged!

garrensmith

Nice that looks cleaner

miguelff force-pushed the feat/find-or-throw branch 3 times, most recently from 8634209 to 82a2bd8 Compare October 31, 2022 16:10

miguelff commented Nov 1, 2022

View reviewed changes

query-engine/core/src/interpreter/query_interpreters/read.rs Outdated Show resolved Hide resolved

miguelff changed the title ~~[WIP] feat(qe): Implement findFirstOrThrow and findUniqeOrThrow natively~~ [WIP] feat(qe): Implement findFirstOrThrow and findUniqeOrThrow in the engine Nov 2, 2022

miguelff added this to the 4.6.0 milestone Nov 2, 2022

miguelff changed the title ~~[WIP] feat(qe): Implement findFirstOrThrow and findUniqeOrThrow in the engine~~ feat(qe): Implement findFirstOrThrow and findUniqeOrThrow in the engine Nov 2, 2022

miguelff requested review from garrensmith and Weakky November 2, 2022 15:37

miguelff marked this pull request as ready for review November 2, 2022 15:38

miguelff commented Nov 2, 2022

View reviewed changes

miguelff mentioned this pull request Nov 3, 2022

WIP: feat: use engine implementation of findFirstOrThrow / findUniqueOrThrow prisma/prisma#16124

Closed

Weakky approved these changes Nov 3, 2022

View reviewed changes

query-engine/schema-builder/src/output_types/query_type.rs Outdated Show resolved Hide resolved

miguelff force-pushed the feat/find-or-throw branch from d8d1b84 to 8481c0d Compare November 4, 2022 09:44

miguelff commented Nov 4, 2022

View reviewed changes

garrensmith suggested changes Nov 4, 2022

View reviewed changes

miguelff requested a review from a team as a code owner November 4, 2022 12:27

miguelff added 6 commits November 4, 2022 14:59

Implement findOneOrThrow, findUniqueOrThrow

3fed088

Reduce complexity and improve readability

0af8b74

Add tests for the new api fields

e9c78ea

Change format, so the model name is infix rather than suffix

a9c2311

User bitflags over a byte, equal footprint than a boolean, and extens…

9ce5fbf

…ible

Hide implementation details of QueryOptions

0dd9c8a

miguelff force-pushed the feat/find-or-throw branch from 2593df8 to 0dd9c8a Compare November 4, 2022 13:59

miguelff requested a review from garrensmith November 4, 2022 15:14

garrensmith approved these changes Nov 4, 2022

View reviewed changes

miguelff merged commit 24b1bcc into main Nov 4, 2022

miguelff deleted the feat/find-or-throw branch November 4, 2022 16:05

miguelff mentioned this pull request Nov 7, 2022

feat: use engine implementation of findFirstOrThrow / findUniqueOrThrow prisma/prisma#16162

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(qe): Implement findFirstOrThrow and findUniqeOrThrow in the engine #3356

feat(qe): Implement findFirstOrThrow and findUniqeOrThrow in the engine #3356

miguelff commented Oct 31, 2022 •

edited

miguelff Nov 2, 2022

miguelff Nov 2, 2022

miguelff Nov 2, 2022

miguelff Nov 2, 2022

miguelff Nov 2, 2022

miguelff Nov 2, 2022

Weakky left a comment

miguelff Nov 4, 2022 •

edited

garrensmith left a comment

garrensmith Nov 4, 2022

garrensmith Nov 4, 2022

miguelff commented Nov 4, 2022 •

edited

miguelff commented Nov 4, 2022

garrensmith left a comment

		2025,
		"An operation failed because it depends on one or more records that were required but not found. Expected a record, found none."

feat(qe): Implement findFirstOrThrow and findUniqeOrThrow in the engine #3356

feat(qe): Implement findFirstOrThrow and findUniqeOrThrow in the engine #3356

Conversation

miguelff commented Oct 31, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Weakky left a comment

Choose a reason for hiding this comment

miguelff Nov 4, 2022 • edited

Choose a reason for hiding this comment

garrensmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelff commented Nov 4, 2022 • edited

miguelff commented Nov 4, 2022

garrensmith left a comment

Choose a reason for hiding this comment

miguelff commented Oct 31, 2022 •

edited

miguelff Nov 4, 2022 •

edited

miguelff commented Nov 4, 2022 •

edited