Resolves domain base types #402

andrew-w-ross · 2023-08-02T14:37:44Z

Closes #399 ~~and potentially #370~~.

Looks like citext type doesn't extend text this pr won't fix it.

I think the coding portion of this is done.
Still need to finish off the domain type tests and the domain array type tests.

Slight deviation is that array types resolve as [base_type] instead of [Opaque].

Edit: I knew that felt too easy, looks like mutation updates have some bugs but for some odd reason mutation insert works fine.

Edit: That was a lot harder then expected a lot of edge cases to deal with.

I'm going to mark this as ready for review but I think there might be potential tests for nullability in domain types.

E.g.

create domain not_null_int as int not null;

create table not_null_test (
    id serial primary key,
    field_int not_null_int
 );

Will resolve with a graphql type of:

type NotNullTest {
 id: Int!
 fieldInt: Int
}

Considering this is a bit of an anti pattern, check the notes section for create domain, it might not be worth dealing with.

sql/load_sql_context.sql

test/expected/domain_field_types.out

test/sql/domain_field_types.sql

test/expected/domain_field_types.out

Runs rollback on domain-test

Resolves base types during sql config load

Removes nightly from docker file for rust

Reverts docker file change

Improved configuration documentation More domain type testing

andrew-w-ross · 2023-09-20T13:30:48Z

@imor Should I update this pr or is there no hope in getting it in?

imor · 2023-09-20T13:42:02Z

Hey @andrew-w-ross I thought this was still in progress. If this was ready, ideally I'd re-request review from the reviewers section at the top right of the PR. Anyways, I've just fixed a conflict and will review by tomorrow.

andrew-w-ross · 2023-09-20T13:48:39Z

@imor Thanks, will do next time

imor

Thanks @andrew-w-ross for this PR. Looking good. Just one question about functions below.

@olirice please take a quick look at this PR, if everything looks good we can merge.

src/sql_types.rs

olirice

great stuff! thanks for putting so much thought into the tests

a few nits but conceptually I'd love to get this merged

olirice · 2023-09-21T18:58:09Z

src/sql_types.rs

+    fn replace_array_element_base_type(mut context: Context) -> Context {
+        for (_, type_) in context.types.iter_mut() {
+            let array_element_type_oid = match type_.array_element_type_oid {
+                Some(oid) => oid,
+                None => continue,
+            };
+
+            let resolve_base_type = context
+                .schemas
+                .get(&type_.schema_oid)
+                .is_some_and(|s| s.directives.resolve_base_type);
+
+            if !resolve_base_type {
+                continue;
+            }
+
+            let array_element_type = match context.base_type_map.get(&array_element_type_oid) {
+                Some(oid) => *oid,
+                None => continue,
+            };
+
+            if let Some(type_) = Arc::get_mut(type_) {
+                type_.array_element_type_oid = Some(array_element_type);
+            }
+        }
+
+        context
+    }
+
+    fn replace_table_base_types(mut context: Context) -> Context {
+        for (_, table) in context.tables.iter_mut() {
+            let resolve_base_type = context
+                .schemas
+                .get(&table.schema_oid)
+                .is_some_and(|s| s.directives.resolve_base_type);
+
+            if !resolve_base_type {
+                continue;
+            }
+
+            let table = match Arc::get_mut(table) {
+                None => continue,
+                Some(table) => table,
+            };
+
+            for column in table.columns.iter_mut() {
+                let base_oid = match context.base_type_map.get(&column.type_oid) {
+                    Some(oid) => *oid,
+                    None => continue,
+                };
+
+                if let Some(column) = Arc::get_mut(column) {
+                    column.type_oid = base_oid;
+                }
+            }
+
+            //Technically speaking, we should check the schema of the function to see if we should resolve the base type
+            //Problem is this might be counter-intuitive to the user as in this case the function is acting as a virtual column of the table
+            for function in table.functions.iter_mut() {
+                let base_oid = match context.base_type_map.get(&function.type_oid) {
+                    Some(oid) => *oid,
+                    None => continue,
+                };
+
+                if let Some(function) = Arc::get_mut(function) {
+                    function.type_oid = base_oid;
+                }
+            }
+        }
+
+        context
+    }
+


How about extending TypeCategory and TypeDetails with a Domain variant and handling the resolution logic in to_graphql_type to keep it consistent with the existing type resolution stuff?

The context is the source of truth for the database' state and I'd like to be able to continue using it confidently for things like casting user input via the SQL layer during transpilation

@olirice Fair point I was trying to change as little as possible initially but that's been thrown out of the window at this point. It makes sense to include it as there is a decent amount more that can be done with that data. I'll get to it this weekend but if you don't mind just review this outline and correct me if I misunderstood something.

Change the base_type_map to domain instead and pull out all relevant, I'm thinking the oid, schema_oid, name, nullability, and possibly the element_oid. Might add the base_oid to that type to help with traversing the types later.

Change the types to include Domain in category type and adding it to the TypeCategory enum. Also create a Domain struct and add it to TypeDetails.

Revert the replace functions in load_sql_context. Mapping the domain type in the type details function.

Finally adding another match block to for Domain in the to_graphql_type function.

As for the tests, you're happy with them as is so if they pass we're good right?

Change the base_type_map to domain instead and pull out all relevant, I'm thinking the oid, schema_oid, name, nullability, and possibly the element_oid. Might add the base_oid to that type to help with traversing the types later.

sorry I didn't understand what you're proposing here. Could you explain the goal at a higher level?

was that related to my comment "How about resolve_domains_as_base_type for clarity?"?
if so, I was only referring to the name of the comment directive. The name of the SQL CTE is okay as-is

Change the types to include Domain in category type and adding it to the TypeCategory enum. Also create a Domain struct and add it to TypeDetails.

yep

Revert the replace functions in load_sql_context. Mapping the domain type in the type details function.

yep

Finally adding another match block to for Domain in the to_graphql_type function.

yep

As for the tests, you're happy with them as is so if they pass we're good right?

they looked good on my first pass. I'll re-review when you're ready but if any need to be adjusted I'm happy to do it

was that related to my comment "How about resolve_domains_as_base_type for clarity?"?

Nope, just changing the config key.

sorry I didn't understand what you're proposing here. Could you explain the goal at a higher level?

Type category tends to map to a type off of the context query. Enum to enums, Composite to composites and Table to tables. I was just going to do the same for domain types. I can add the base_oid under table_oid. That's all the info I need to get this done for now. I was just thinking ahead for all the other data you might need for domains, like is it nullable or is it an array, etc.

sql/load_sql_context.sql

docs/configuration.md

olirice · 2023-09-21T19:35:27Z

test/expected/domain_field_types.out

+     }                                                       +
+ }
+(1 row)
+


Could you add a tests that show what kind of error output we get if a user provides an invalid input value for a domain field?

For example, given a pos_int domain:

what happens if the user enters a negative number in a Filter

what happens if the user tries to insert a negative value

Will do, Is this just for documenting behavior or is there a specific expected behavior other then returning:

{ "data": null, "errors": [ { "message": "Something bad happened." } ] }

just for documenting behavior, but if its too generic I might make a note to improve in the future

olirice · 2023-10-03T13:07:08Z

@andrew-w-ross I've converted this to a draft to keep the PRs clear. Please mark it "ready for review" once your ready

olirice · 2023-10-13T14:07:43Z

closing this for now. please re-open when you're ready

imor reviewed Aug 3, 2023

View reviewed changes

andrew-w-ross added 5 commits August 9, 2023 08:46

Resolves domain base types

b8504d6

Fixes ci issues

bbd9452

Runs rollback on domain-test

resolve_base_type defaults to false

3076c7f

Reverts changes to graphql.rs

79edc5a

Resolves base types during sql config load

FilterType now filters invalid types using oid instead of name

54f2349

andrew-w-ross force-pushed the feature/domain-types branch from 3f534bb to 54f2349 Compare August 13, 2023 20:58

andrew-w-ross added 5 commits August 14, 2023 08:16

Adds docs for resolve_base_type

ec2afb2

Removes nightly from docker file for rust

Corrects delete mutation call

3933fcf

Reverts docker file change

Resolves array element base type

c1c9564

Improved configuration documentation More domain type testing

Commit check

6dc1c10

Minor cleanup

055b919

andrew-w-ross marked this pull request as ready for review August 17, 2023 04:19

Merge branch 'master' into feature/domain-types

665fa3f

imor added 4 commits September 21, 2023 10:46

remove *.session.sql from .gitignore

ec1c3df

improve docs

6549d91

add a note to the docs

e930a60

use consts instead of magic numbers

cbe8b99

imor reviewed Sep 21, 2023

View reviewed changes

src/sql_types.rs Show resolved Hide resolved

imor added 2 commits September 21, 2023 13:52

update changelog

7e4ee07

fix pre-commit failure

12b7aef

olirice requested changes Sep 21, 2023

View reviewed changes

Marviel mentioned this pull request Sep 22, 2023

Treat citext as String rather Opaque #370

Closed

olirice marked this pull request as draft October 3, 2023 13:05

olirice closed this Oct 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolves domain base types #402

Resolves domain base types #402

andrew-w-ross commented Aug 2, 2023 •

edited

Loading

andrew-w-ross commented Sep 20, 2023

imor commented Sep 20, 2023

andrew-w-ross commented Sep 20, 2023

imor left a comment •

edited

Loading

olirice left a comment

olirice Sep 21, 2023

andrew-w-ross Sep 22, 2023 •

edited

Loading

olirice Sep 22, 2023

andrew-w-ross Sep 22, 2023 •

edited

Loading

olirice Sep 21, 2023

andrew-w-ross Sep 22, 2023

olirice Sep 22, 2023

olirice commented Oct 3, 2023

olirice commented Oct 13, 2023

Resolves domain base types #402

Resolves domain base types #402

Conversation

andrew-w-ross commented Aug 2, 2023 • edited Loading

andrew-w-ross commented Sep 20, 2023

imor commented Sep 20, 2023

andrew-w-ross commented Sep 20, 2023

imor left a comment • edited Loading

Choose a reason for hiding this comment

olirice left a comment

Choose a reason for hiding this comment

olirice Sep 21, 2023

Choose a reason for hiding this comment

andrew-w-ross Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

olirice Sep 22, 2023

Choose a reason for hiding this comment

andrew-w-ross Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

olirice Sep 21, 2023

Choose a reason for hiding this comment

andrew-w-ross Sep 22, 2023

Choose a reason for hiding this comment

olirice Sep 22, 2023

Choose a reason for hiding this comment

olirice commented Oct 3, 2023

olirice commented Oct 13, 2023

andrew-w-ross commented Aug 2, 2023 •

edited

Loading

imor left a comment •

edited

Loading

andrew-w-ross Sep 22, 2023 •

edited

Loading

andrew-w-ross Sep 22, 2023 •

edited

Loading