Merge pull request #1938 from MohamedSabthar/cahce-changes

Remove caching every field for records with non-optional fields

ballerina-tests/Ballerina.toml

@@ -1,7 +1,7 @@
 [package]
 org = "ballerina"
 name = "graphql_tests"
-version = "1.13.0"
+version = "1.13.1"
 
 [build-options]
 observabilityIncluded = true

ballerina-tests/Dependencies.toml

@@ -67,7 +67,7 @@ modules = [
 [[package]]
 org = "ballerina"
 name = "graphql"
-version = "1.13.0"
+version = "1.13.1"
 dependencies = [
 	{org = "ballerina", name = "auth"},
 	{org = "ballerina", name = "cache"},
@@ -99,7 +99,7 @@ modules = [
 [[package]]
 org = "ballerina"
 name = "graphql_tests"
-version = "1.13.0"
+version = "1.13.1"
 dependencies = [
 	{org = "ballerina", name = "constraint"},
 	{org = "ballerina", name = "file"},

ballerina-tests/tests/44_server_caches.bal

@@ -198,6 +198,34 @@ isolated function testServerSideCacheInOperationalLevelWithTTL() returns error?
     assertJsonValuesWithOrder(actualPayload, expectedPayload);
 }
 
+@test:Config {
+    groups: ["server_cache"]
+}
+isolated function testCachingRecordWithoutNonOptionalFields() returns error? {
+    string url = "http://localhost:9091/server_cache_records_with_non_optional";
+    string document = check getGraphqlDocumentFromFile("server_cache_records_with_non_optional_fields_1");
+
+    json actualPayload = check getJsonPayloadFromService(url, document, (), "GetProfiles");
+    json expectedPayload = check getJsonContentFromFile("server_cache_records_with_non_optional_fields_1");
+    assertJsonValuesWithOrder(actualPayload, expectedPayload);
+
+    _ = check getJsonPayloadFromService(url, document, {enableEvict: false}, "RemoveProfiles");
+
+    actualPayload = check getJsonPayloadFromService(url, document, (), "GetProfiles");
+    assertJsonValuesWithOrder(actualPayload, expectedPayload);
+
+    document = check getGraphqlDocumentFromFile("server_cache_records_with_non_optional_fields_2");
+    actualPayload = check getJsonPayloadFromService(url, document, (), "GetProfiles");
+    expectedPayload = check getJsonContentFromFile("server_cache_records_with_non_optional_fields_2");
+    assertJsonValuesWithOrder(actualPayload, expectedPayload);
+
+    _ = check getJsonPayloadFromService(url, document, {enableEvict: true}, "RemoveProfiles");
+
+    actualPayload = check getJsonPayloadFromService(url, document, (), "GetProfiles");
+    expectedPayload = check getJsonContentFromFile("server_cache_records_with_non_optional_fields_3");
+    assertJsonValuesWithOrder(actualPayload, expectedPayload);
+}
+
 @test:Config {
     groups: ["server_cache"]
 }

ballerina-tests/tests/records.bal

@@ -407,3 +407,9 @@ type User record {|
     string name?;
     int age?;
 |};
+
+type ProfileInfo record {|
+    string name;
+    int age;
+    string contact;
+|};

ballerina-tests/tests/resources/documents/server_cache_records_with_non_optional_fields_1.graphql

@@ -0,0 +1,11 @@
+query GetProfiles {
+  profiles {
+    name
+  }
+}
+
+mutation RemoveProfiles($enableEvict: Boolean!) {
+  removeProfiles(enableEvict: $enableEvict) {
+    name
+  }
+}

ballerina-tests/tests/resources/documents/server_cache_records_with_non_optional_fields_2.graphql

@@ -0,0 +1,12 @@
+query GetProfiles {
+  profiles {
+    name
+    age
+  }
+}
+
+mutation RemoveProfiles($enableEvict: Boolean!) {
+  removeProfiles(enableEvict: $enableEvict) {
+    name
+  }
+}

ballerina-tests/tests/resources/expected_results/server_cache_records_with_non_optional_fields_1.json

@@ -0,0 +1,15 @@
+{
+    "data": {
+        "profiles": [
+            {
+                "name": "John"
+            },
+            {
+                "name": "Doe"
+            },
+            {
+                "name": "Jane"
+            }
+        ]
+    }
+}

ballerina-tests/tests/resources/expected_results/server_cache_records_with_non_optional_fields_2.json

@@ -0,0 +1,18 @@
+{
+    "data": {
+        "profiles": [
+            {
+                "name": "John",
+                "age": 30
+            },
+            {
+                "name": "Doe",
+                "age": 25
+            },
+            {
+                "name": "Jane",
+                "age": 35
+            }
+        ]
+    }
+}

ballerina-tests/tests/resources/expected_results/server_cache_records_with_non_optional_fields_3.json

@@ -0,0 +1,5 @@
+{
+    "data": {
+        "profiles": []
+    }
+}

ballerina-tests/tests/test_services.bal

@@ -3044,3 +3044,30 @@ service /cache_with_list_input on basicListener {
         return address;
     }
 }
+
+
+@graphql:ServiceConfig {
+    cacheConfig: {
+        maxSize: 5
+    }
+}
+service /server_cache_records_with_non_optional on basicListener {
+    private table<ProfileInfo> profiles = table [
+        {name: "John", age: 30, contact: "0123456789"},
+        {name: "Doe", age: 25, contact: "9876543210"},
+        {name: "Jane", age: 35, contact: "1234567890"}
+    ];
+
+    resource function get profiles() returns ProfileInfo[] {
+        return self.profiles.toArray();
+    }
+
+    remote function removeProfiles(graphql:Context ctx, boolean enableEvict) returns ProfileInfo[]|error {
+        if enableEvict {
+            check ctx.invalidateAll();
+        }
+        ProfileInfo[] profiles = self.profiles.toArray();
+        self.profiles.removeAll();
+        return profiles;
+    }
+}

ballerina/Ballerina.toml

@@ -1,7 +1,7 @@
 [package]
 org = "ballerina"
 name = "graphql"
-version = "1.13.0"
+version = "1.13.1"
 authors = ["Ballerina"]
 export=["graphql", "graphql.subgraph", "graphql.dataloader"]
 keywords = ["gql", "network", "query", "service"]
@@ -16,11 +16,11 @@ graalvmCompatible = true
 [[platform.java17.dependency]]
 groupId = "io.ballerina.stdlib"
 artifactId = "graphql-native"
-version = "1.13.0"
-path = "../native/build/libs/graphql-native-1.13.0.jar"
+version = "1.13.1"
+path = "../native/build/libs/graphql-native-1.13.1-SNAPSHOT.jar"
 
 [[platform.java17.dependency]]
 groupId = "io.ballerina.stdlib"
 artifactId = "graphql-commons"
-version = "1.13.0"
-path = "../commons/build/libs/graphql-commons-1.13.0.jar"
+version = "1.13.1"
+path = "../commons/build/libs/graphql-commons-1.13.1-SNAPSHOT.jar"

ballerina/CompilerPlugin.toml

@@ -3,7 +3,7 @@ id = "graphql-compiler-plugin"
 class = "io.ballerina.stdlib.graphql.compiler.GraphqlCompilerPlugin"
 
 [[dependency]]
-path = "../compiler-plugin/build/libs/graphql-compiler-plugin-1.13.0.jar"
+path = "../compiler-plugin/build/libs/graphql-compiler-plugin-1.13.1-SNAPSHOT.jar"
 
 [[dependency]]
-path = "../commons/build/libs/graphql-commons-1.13.0.jar"
+path = "../commons/build/libs/graphql-commons-1.13.1-SNAPSHOT.jar"

ballerina/Dependencies.toml

@@ -73,7 +73,7 @@ modules = [
 [[package]]
 org = "ballerina"
 name = "graphql"
-version = "1.13.0"
+version = "1.13.1"
 dependencies = [
 	{org = "ballerina", name = "auth"},
 	{org = "ballerina", name = "cache"},

ballerina/context.bal

@@ -22,10 +22,10 @@ import ballerina/lang.value;
 
 # The GraphQL context object used to pass the meta information between resolvers.
 public isolated class Context {
-    private final map<value:Cloneable|isolated object {}> attributes;
-    private final ErrorDetail[] errors;
-    private Engine? engine;
-    private int nextInterceptor;
+    private final map<value:Cloneable|isolated object {}> attributes = {};
+    private final ErrorDetail[] errors = [];
+    private Engine? engine = ();
+    private int nextInterceptor = 0;
     private boolean hasFileInfo = false; // This field value changed by setFileInfo method
     private map<dataloader:DataLoader> idDataLoaderMap = {}; // Provides mapping between user defined id and DataLoader
     private map<Placeholder> uuidPlaceholderMap = {};
@@ -34,20 +34,6 @@ public isolated class Context {
     private int unResolvedPlaceholderCount = 0; // Tracks the number of Placeholders needs to be resolved
     private int unResolvedPlaceholderNodeCount = 0; // Tracks the number of nodes to be replaced in the value tree
 
-    public isolated function init(map<value:Cloneable|isolated object {}> attributes = {}, Engine? engine = (),
-                                  int nextInterceptor = 0) {
-        self.attributes = {};
-        self.engine = engine;
-        self.errors = [];
-        self.nextInterceptor = nextInterceptor;
-
-        foreach var [key, value] in attributes.entries() {
-            lock {
-                self.attributes[key] = value is value:Cloneable ? value.cloneReadOnly() : value;
-            }
-        }
-    }
-
     # Sets a given value for a given key in the GraphQL context.
     #
     # + key - The key for the value to be set

ballerina/engine.bal

@@ -63,7 +63,10 @@ isolated class Engine {
         return self.cacheConfig;
     }
 
-    isolated function addToCache(string key, any value, decimal maxAge) returns any|error {
+    isolated function addToCache(string key, any value, decimal maxAge, boolean alreadyCached = false) returns any|error {
+        if alreadyCached {
+            return;
+        }
         cache:Cache? cache = self.cache;
         if cache is cache:Cache {
             return cache.put(key, value, maxAge);
@@ -314,7 +317,7 @@ isolated class Engine {
             }
             any fieldValue;
             parser:RootOperationType operationType = 'field.getOperationType();
-            if operationType == parser:OPERATION_QUERY && 'field.isCacheEnabled() {
+            if 'field.isCacheEnabled() && 'field.getOperationType() == parser:OPERATION_QUERY {
                 string cacheName = string `${'field.getName()}.cache`;
                 addTracingInfomation({context, serviceName: cacheName, operationType});
                 addFieldMetric('field);
@@ -325,8 +328,9 @@ isolated class Engine {
                 } else {
                     fieldValue = check self.getFieldValue(context, 'field, responseGenerator);
                     decimal maxAge = 'field.getCacheMaxAge();
-                    if maxAge > 0d && fieldValue !is () {
-                        _ = check self.addToCache(cacheKey, fieldValue, maxAge);
+                    boolean alreadyCached = 'field.isAlreadyCached();
+                    if !alreadyCached && maxAge > 0d && fieldValue !is () {
+                        _ = check self.addToCache(cacheKey, fieldValue, maxAge, alreadyCached);
                     }
                 }
             } else {

ballerina/engine_utils.bal

@@ -136,3 +136,7 @@ isolated function hasRecordReturnType(service object {} serviceObject, string[]
     returns boolean = @java:Method {
     'class: "io.ballerina.stdlib.graphql.runtime.engine.Engine"
 } external;
+
+isolated function isRecordWithNoOptionalFields(any|error value) returns boolean = @java:Method {
+    'class: "io.ballerina.stdlib.graphql.runtime.engine.EngineUtils"
+} external;

ballerina/field.bal

@@ -32,11 +32,12 @@ public class Field {
     private final boolean cacheEnabled;
     private final decimal cacheMaxAge;
     private boolean hasRequestedNullableFields;
+    private final boolean alreadyCached;
 
     isolated function init(parser:FieldNode internalNode, __Type fieldType, __Type parentType,
             service object {}? serviceObject = (), (string|int)[] path = [],
             parser:RootOperationType operationType = parser:OPERATION_QUERY, string[] resourcePath = [],
-            any|error fieldValue = (), ServerCacheConfig? cacheConfig = (), readonly & string[] parentArgHashes = []) {
+            any|error fieldValue = (), ServerCacheConfig? cacheConfig = (), readonly & string[] parentArgHashes = [], boolean isAlreadyCached = false) {
         self.internalNode = internalNode;
         self.serviceObject = serviceObject;
         self.fieldType = fieldType;
@@ -46,6 +47,7 @@ public class Field {
         self.resourcePath = resourcePath;
         self.fieldValue = fieldValue;
         self.resourcePath.push(internalNode.getName());
+        self.alreadyCached = isAlreadyCached;
         self.fieldInterceptors = serviceObject is service object {} ?
             getFieldInterceptors(serviceObject, operationType, internalNode.getName(), self.resourcePath) : [];
         ServerCacheConfig? fieldCache = serviceObject is service object {} ?
@@ -136,6 +138,10 @@ public class Field {
         return self.fieldType;
     }
 
+    isolated function isAlreadyCached() returns boolean {
+        return self.alreadyCached;
+    }
+
     isolated function getFieldValue() returns any|error {
         return self.fieldValue;
     }
@@ -155,9 +161,13 @@ public class Field {
             if selection is parser:FieldNode {
                 foreach __Field 'field in typeFields {
                     if 'field.name == selection.getName() {
-                        result.push(new Field(selection, 'field.'type, parentType, (),[...currentPath, ...unwrappedPath, 'field.name],
-                            self.operationType.clone(), self.resourcePath.clone(), cacheConfig = self.cacheConfig,
-                            parentArgHashes = self.parentArgHashes));
+                        result.push(new Field(selection, 'field.'type, parentType, (), [
+                                ...currentPath,
+                                ...unwrappedPath,
+                                'field.name
+                            ], self.operationType.clone(), self.resourcePath.clone(),
+                            cacheConfig = self.cacheConfig, parentArgHashes = self.parentArgHashes
+                        ));
                         break;
                     }
                 }

ballerina/response_generator.bal

@@ -154,11 +154,12 @@ class ResponseGenerator {
         any fieldValue = parentValue.hasKey(fieldNode.getName()) ? parentValue.get(fieldNode.getName()) : ();
         __Type parentType = self.fieldType;
         __Type fieldType = getFieldTypeFromParentType(parentType, self.engine.getSchema().types, fieldNode);
+        boolean isAlreadyCached = isRecordWithNoOptionalFields(parentValue);
         (string|int)[] clonedPath = self.path.clone();
         clonedPath.push(fieldNode.getAlias());
         Field 'field = new (fieldNode, fieldType, parentType, path = clonedPath, fieldValue = fieldValue,
-            cacheConfig = self.cacheConfig, parentArgHashes = self.parentArgHashes
-        );
+            cacheConfig = self.cacheConfig, parentArgHashes = self.parentArgHashes,
+            isAlreadyCached = isAlreadyCached);
         self.context.resetInterceptorCount();
         return self.engine.resolve(self.context, 'field);
     }

ballerina/tests/09_cache_utils.bal

@@ -13,9 +13,9 @@
 // KIND, either express or implied.  See the License for the
 // specific language governing permissions and limitations
 // under the License.
+import graphql.parser;
 
 import ballerina/test;
-import graphql.parser;
 
 @test:Config {
     groups: ["server_cache"]
@@ -39,7 +39,7 @@ function testCacheConfigInferring() returns error? {
     test:assertTrue('field.getSubfields() is Field[]);
     Field[] subfields = <Field[]>'field.getSubfields();
     string[] expectedCacheKey = ["person.name.11FxOYiYfpMxmANj4kGJzg==", "person.address.nj4v+q6cUjv3W/MbZdNQXg=="];
-    foreach int i in 0..<subfields.length() {
+    foreach int i in 0 ..< subfields.length() {
         test:assertTrue(subfields[i].isCacheEnabled());
         test:assertEquals(subfields[i].getCacheMaxAge(), 10d);
         test:assertEquals(subfields[i].getCacheKey(), expectedCacheKey[i]);

changelog.md

@@ -6,6 +6,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 
 ## [Unreleased]
 
+### Changed
+- [[#6652] Cache Entire Record Having All Non-Optional Fields Instead of Caching Each Field Separately](https://github.com/ballerina-platform/ballerina-library/issues/6652)
+
 ## [1.13.0] - 2024-05-06
 
 ### Added