Use type rather than model names in JSONAPI sparse fields (hasura#1349)

### What Given a model name `Authors` with an `Author` type underneath, this changes field selection for JSONAPI to use `fields[Author]` instead of `fields[Authors]`. This will make things more consistent when we are also filtering other types from relationships or nested fields too, which will have to use type names, for example `fields[Author]=name,articles&fields[Article]=title` V3_GIT_ORIGIN_REV_ID: fff43c673888877ca8b2edb6e5ca9fdc846675a5
chardo · Nov 15, 2024 · bf61f3e · bf61f3e
1 parent 5c7e20f
commit bf61f3e
Show file tree

Hide file tree

Showing 10 changed files with 52 additions and 57 deletions.
diff --git a/v3/crates/jsonapi/src/parse.rs b/v3/crates/jsonapi/src/parse.rs
@@ -5,7 +5,7 @@ use open_dds::{
     identifier,
     identifier::{Identifier, SubgraphName},
     models::ModelName,
-    types::FieldName,
+    types::{CustomTypeName, FieldName},
 };
 use serde::{Deserialize, Serialize};
 mod filter;
@@ -38,7 +38,7 @@ pub fn create_query_ir(
     // create the selection fields; include all fields of the model output type
     let mut selection = IndexMap::new();
     for field_name in model.type_fields.keys() {
-        if include_field(query_string, field_name, &model.name.name) {
+        if include_field(query_string, field_name, &model.data_type.name) {
             let field_name_ident = Identifier::new(field_name.as_str())
                 .map_err(|e| RequestError::BadRequest(e.into()))?;
 
@@ -118,23 +118,23 @@ fn validate_sparse_fields(
     model: &Model,
     query_string: &jsonapi_library::query::Query,
 ) -> Result<(), RequestError> {
-    let model_name_string = model.name.name.to_string();
+    let type_name_string = model.data_type.name.to_string();
     if let Some(fields) = &query_string.fields {
-        for (model_name, model_fields) in fields {
-            if *model_name == model_name_string {
-                for models_field in model_fields {
+        for (type_name, type_fields) in fields {
+            if *type_name == type_name_string {
+                for type_field in type_fields {
                     let string_fields: Vec<_> =
                         model.type_fields.keys().map(ToString::to_string).collect();
 
-                    if !string_fields.contains(models_field) {
+                    if !string_fields.contains(type_field) {
                         return Err(RequestError::BadRequest(format!(
-                            "Unknown field in sparse fields: {models_field}"
+                            "Unknown field in sparse fields: {type_field}"
                         )));
                     }
                 }
             } else {
                 return Err(RequestError::BadRequest(format!(
-                    "Unknown model in sparse fields: {model_name}"
+                    "Unknown type in sparse fields: {type_name}"
                 )));
             }
         }
@@ -143,30 +143,25 @@ fn validate_sparse_fields(
 }
 
 // given the sparse fields for this request, should be include a given field in the query?
-// this does not consider subgraphs at the moment - we match on `ModelName` not
-// `Qualified<ModelName>`.
-// This means that the below field is ambiguous where `Authors` model is defined in multiple
+// this does not consider subgraphs at the moment - we match on `CustomTypeName` not
+// `Qualified<CustomTypeName>`.
+// This means that the below field is ambiguous where `Authors` type is defined in multiple
 // subgraphs
 // fields[Authors]=author_id,first_name
 //
-// two possible solutions:
-// 1. make users qualify the name inline
-//
-// fields[subgraph.Authors]=author_id,first_name&fields[other.Authors]=author_id,last_name
-//
-// 2. much like we make users explicitly give GraphQL names to things, we
-// make them give JSONAPI models an unambiguous name in metadata, and the user provides that:
+// This will need to be solved when we make users give JSONAPI types explicit names
+// like we do in GraphQL
 //
 // fields[subgraphAuthors]=author_id,firstName&fields[otherAuthors]=author_id,last_name
 fn include_field(
     query_string: &jsonapi_library::query::Query,
     field_name: &FieldName,
-    model_name: &ModelName,
+    object_type_name: &CustomTypeName,
 ) -> bool {
     if let Some(fields) = &query_string.fields {
-        if let Some(model_fields) = fields.get(model_name.as_str()) {
-            for model_field in model_fields {
-                if model_field == field_name.as_str() {
+        if let Some(object_fields) = fields.get(object_type_name.0.as_str()) {
+            for object_field in object_fields {
+                if object_field == field_name.as_str() {
                     return true;
                 }
             }

diff --git a/v3/crates/jsonapi/src/schema/parameters.rs b/v3/crates/jsonapi/src/schema/parameters.rs
@@ -60,7 +60,7 @@ pub fn fields_parameter(model: &Model) -> oas3::spec::Parameter {
     }
 
     oas3::spec::Parameter {
-        name: format!("fields[{}]", model.name.name),
+        name: format!("fields[{}]", model.data_type.name),
         allow_empty_value: None,
         allow_reserved: None,
         content: None,

diff --git a/v3/crates/jsonapi/tests/failing/select_model/Authors.txt b/v3/crates/jsonapi/tests/failing/select_model/Authors.txt
@@ -1 +1 @@
-fields[Authors]=author_id,first_name,last_name&page[offset]=0&page[limit]=1
+fields[Author]=author_id,first_name,last_name&page[offset]=0&page[limit]=1
diff --git a/v3/crates/jsonapi/tests/jsonapi_golden_tests.rs b/v3/crates/jsonapi/tests/jsonapi_golden_tests.rs
@@ -24,7 +24,7 @@ fn test_get_succeeding_requests() {
             let TestRequest { query, model_name } = test_request_setup(path);
 
             // always test in `default` subgraph for now
-            let path = format!("/default/{model_name}");
+            let request_path = format!("/default/{model_name}");
 
             let http_context = execute::HttpContext {
                 client: reqwest::Client::new(),
@@ -40,7 +40,7 @@ fn test_get_succeeding_requests() {
                 &jsonapi_catalog,
                 metadata.into(),
                 axum::http::method::Method::GET,
-                axum::http::uri::Uri::from_str(&path).unwrap(),
+                axum::http::uri::Uri::from_str(&request_path).unwrap(),
                 query,
             )
             .await;
@@ -52,7 +52,7 @@ fn test_get_succeeding_requests() {
                         result
                     );
                 }
-                Err(e) => panic!("expected success, instead got {e}"),
+                Err(e) => panic!("expected success for {path:?}, instead got {e}"),
             }
         });
     });
@@ -75,7 +75,7 @@ fn test_get_failing_requests() {
             let TestRequest { query, model_name } = test_request_setup(path);
 
             // always test in `default` subgraph for now
-            let path = format!("/default/{model_name}");
+            let request_path = format!("/default/{model_name}");
 
             let http_context = execute::HttpContext {
                 client: reqwest::Client::new(),
@@ -91,7 +91,7 @@ fn test_get_failing_requests() {
                 &jsonapi_catalog,
                 metadata.into(),
                 axum::http::method::Method::GET,
-                axum::http::uri::Uri::from_str(&path).unwrap(),
+                axum::http::uri::Uri::from_str(&request_path).unwrap(),
                 query,
             )
             .await;

diff --git a/v3/crates/jsonapi/tests/passing/select_model/Authors.txt b/v3/crates/jsonapi/tests/passing/select_model/Authors.txt
@@ -1 +1 @@
-fields[Authors]=first_name&page[offset]=1&page[limit]=4
+fields[Author]=first_name&page[offset]=1&page[limit]=4
diff --git a/...rates/jsonapi/tests/snapshots/jsonapi_golden_tests__generated_openapi_for_role_admin.snap b/...rates/jsonapi/tests/snapshots/jsonapi_golden_tests__generated_openapi_for_role_admin.snap
@@ -122,7 +122,7 @@ expression: generated_openapi
     },
     "/v1/rest/default/Articles": {
       "get": {
-        "summary": "Fetch article values",
+        "summary": "Fetch Article values",
         "parameters": [
           {
             "name": "page[limit]",
@@ -143,7 +143,7 @@ expression: generated_openapi
             "example": "10"
           },
           {
-            "name": "fields[Articles]",
+            "name": "fields[Article]",
             "in": "query",
             "description": "Optional list of fields from Articles to include in response. If no fields are provided, all fields are returned",
             "schema": {
@@ -199,7 +199,7 @@ expression: generated_openapi
                         "properties": {
                           "_type": {
                             "enum": [
-                              "default_article"
+                              "default_Article"
                             ]
                           },
                           "attributes": {
@@ -336,7 +336,7 @@ expression: generated_openapi
     },
     "/v1/rest/default/Authors": {
       "get": {
-        "summary": "Fetch author values",
+        "summary": "Fetch Author values",
         "description": "top level Authors model description",
         "parameters": [
           {
@@ -358,7 +358,7 @@ expression: generated_openapi
             "example": "10"
           },
           {
-            "name": "fields[Authors]",
+            "name": "fields[Author]",
             "in": "query",
             "description": "Optional list of fields from Authors to include in response. If no fields are provided, all fields are returned",
             "schema": {
@@ -411,7 +411,7 @@ expression: generated_openapi
                         "properties": {
                           "_type": {
                             "enum": [
-                              "default_author"
+                              "default_Author"
                             ]
                           },
                           "attributes": {
@@ -1644,7 +1644,7 @@ expression: generated_openapi
             "example": "10"
           },
           {
-            "name": "fields[institutions]",
+            "name": "fields[institution]",
             "in": "query",
             "description": "Optional list of fields from institutions to include in response. If no fields are provided, all fields are returned",
             "schema": {

diff --git a/...ates/jsonapi/tests/snapshots/jsonapi_golden_tests__generated_openapi_for_role_user_1.snap b/...ates/jsonapi/tests/snapshots/jsonapi_golden_tests__generated_openapi_for_role_user_1.snap
@@ -116,7 +116,7 @@ expression: generated_openapi
     },
     "/v1/rest/default/Articles": {
       "get": {
-        "summary": "Fetch article values",
+        "summary": "Fetch Article values",
         "parameters": [
           {
             "name": "page[limit]",
@@ -137,7 +137,7 @@ expression: generated_openapi
             "example": "10"
           },
           {
-            "name": "fields[Articles]",
+            "name": "fields[Article]",
             "in": "query",
             "description": "Optional list of fields from Articles to include in response. If no fields are provided, all fields are returned",
             "schema": {
@@ -190,7 +190,7 @@ expression: generated_openapi
                         "properties": {
                           "_type": {
                             "enum": [
-                              "default_article"
+                              "default_Article"
                             ]
                           },
                           "attributes": {

diff --git a/...ates/jsonapi/tests/snapshots/jsonapi_golden_tests__generated_openapi_for_role_user_2.snap b/...ates/jsonapi/tests/snapshots/jsonapi_golden_tests__generated_openapi_for_role_user_2.snap
@@ -12,7 +12,7 @@ expression: generated_openapi
   "paths": {
     "/v1/rest/default/Articles": {
       "get": {
-        "summary": "Fetch article values",
+        "summary": "Fetch Article values",
         "parameters": [
           {
             "name": "page[limit]",
@@ -33,7 +33,7 @@ expression: generated_openapi
             "example": "10"
           },
           {
-            "name": "fields[Articles]",
+            "name": "fields[Article]",
             "in": "query",
             "description": "Optional list of fields from Articles to include in response. If no fields are provided, all fields are returned",
             "schema": {
@@ -86,7 +86,7 @@ expression: generated_openapi
                         "properties": {
                           "_type": {
                             "enum": [
-                              "default_article"
+                              "default_Article"
                             ]
                           },
                           "attributes": {

diff --git a/...ests/snapshots/jsonapi_golden_tests__result_for_role_admin@select_model__Authors.txt.snap b/...ests/snapshots/jsonapi_golden_tests__result_for_role_admin@select_model__Authors.txt.snap
@@ -8,7 +8,7 @@ DocumentData {
         Multiple(
             [
                 Resource {
-                    _type: "default_author",
+                    _type: "default_Author",
                     id: "1",
                     attributes: {
                         "first_name": String("John"),

diff --git a/v3/crates/jsonapi/tests/static/metadata.json b/v3/crates/jsonapi/tests/static/metadata.json
@@ -2057,7 +2057,7 @@
           "kind": "ObjectType",
           "version": "v1",
           "definition": {
-            "name": "article",
+            "name": "Article",
             "fields": [
               {
                 "name": "article_id",
@@ -2108,7 +2108,7 @@
             "name": "article_bool_exp",
             "operand": {
               "object": {
-                "type": "article",
+                "type": "Article",
                 "comparableFields": [
                   {
                     "fieldName": "author_id",
@@ -2141,7 +2141,7 @@
           "kind": "TypePermissions",
           "version": "v1",
           "definition": {
-            "typeName": "article",
+            "typeName": "Article",
             "permissions": [
               {
                 "role": "admin",
@@ -2168,7 +2168,7 @@
           "kind": "ObjectType",
           "version": "v1",
           "definition": {
-            "name": "author",
+            "name": "Author",
             "description": "author description",
             "fields": [
               {
@@ -2221,7 +2221,7 @@
             "name": "author_bool_exp",
             "operand": {
               "object": {
-                "type": "author",
+                "type": "Author",
                 "comparableFields": [
                   {
                     "fieldName": "author_id",
@@ -2254,7 +2254,7 @@
           "kind": "TypePermissions",
           "version": "v1",
           "definition": {
-            "typeName": "author",
+            "typeName": "Author",
             "permissions": [
               {
                 "role": "admin",
@@ -2270,7 +2270,7 @@
           "version": "v1",
           "definition": {
             "name": "Articles",
-            "objectType": "article",
+            "objectType": "Article",
             "globalIdSource": true,
             "source": {
               "dataConnectorName": "db",
@@ -2376,7 +2376,7 @@
           "version": "v1",
           "definition": {
             "name": "Authors",
-            "objectType": "author",
+            "objectType": "Author",
             "description": "top level Authors model description",
             "globalIdSource": true,
             "source": {
@@ -5306,7 +5306,7 @@
         },
         {
           "definition": {
-            "sourceType": "article",
+            "sourceType": "Article",
             "name": "AuthorFromCommand",
             "description": "AuthorFromCommand description",
             "target": {
@@ -5338,8 +5338,8 @@
           "kind": "Relationship",
           "version": "v1",
           "definition": {
-            "sourceType": "author",
-            "name": "Articles",
+            "sourceType": "Author",
+            "name": "articles",
             "target": {
               "model": {
                 "name": "Articles",
@@ -5370,7 +5370,7 @@
           "kind": "Relationship",
           "version": "v1",
           "definition": {
-            "sourceType": "article",
+            "sourceType": "Article",
             "name": "Author",
             "target": {
               "model": {
Original file line number	Diff line number	Diff line change
		@@ -1 +1 @@
		fields[Authors]=author_id,first_name,last_name&page[offset]=0&page[limit]=1
		fields[Author]=author_id,first_name,last_name&page[offset]=0&page[limit]=1
Original file line number	Diff line number	Diff line change
		@@ -1 +1 @@
		fields[Authors]=first_name&page[offset]=1&page[limit]=4
		fields[Author]=first_name&page[offset]=1&page[limit]=4