Return meaningful values for MessageTask getMethodName function[API-2037] #25020

ihsandemir · 2023-07-14T15:25:00Z

Return meaningful values for MessageTask getMethodName function. Added non-null method names to some of the message tasks which were missing the names and can be used in security interceptor since they are related to the user method calls.

The ones which return null for the getMethodName function are the internally used or not directly related to the user API calls.

Also, removed the default null returning implementations in some of the base message tasks.

EE PR: https://github.com/hazelcast/hazelcast-enterprise/pull/6252

ihsandemir · 2023-07-14T15:26:41Z

The following is the list of message tasks which will continue returning null after this PR is merged.

Targets
    Occurrences of '    public String getMethodName\(\) \{.*
    .*return null;
    \}' in Directory /Users/ihsan/Desktop/work/src/hazelcast
Found Occurrences in Directory /Users/ihsan/Desktop/work/src/hazelcast  (34 usages found)
    Production  (34 usages found)
        Unclassified  (34 usages found)
            hazelcast  (34 usages found)
                com.hazelcast.client.impl.protocol.task  (12 usages found)
                    AddBackupListenerMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            89 public String getMethodName() {
                    AddClusterViewListenerMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            74 public String getMethodName() {
                    AuthenticationBaseMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            256 public String getMethodName() {
                    ClientStatisticsMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            68 public String getMethodName() {
                    CreateProxiesMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            98 public String getMethodName() {
                    CreateProxyMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            98 public String getMethodName() {
                    DeployClassesMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            93 public String getMethodName() {
                    DestroyProxyMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            104 public String getMethodName() {
                    ExperimentalTpcAuthenticationMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            59 public String getMethodName() {
                    NoSuchMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            64 public String getMethodName() {
                    PingMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            68 public String getMethodName() {
                    TriggerPartitionAssignmentMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            67 public String getMethodName() {
                com.hazelcast.client.impl.protocol.task.cache  (6 usages found)
                    CacheAddNearCacheInvalidationListenerTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            105 public String getMethodName() {
                    CacheDestroyMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            78 public String getMethodName() {
                    CacheFetchNearCacheInvalidationMetadataTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            78 public String getMethodName() {
                    CacheGetConfigMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            62 public String getMethodName() {
                    CacheManagementConfigMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            79 public String getMethodName() {
                    CacheRemoveInvalidationListenerMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            79 public String getMethodName() {
                com.hazelcast.client.impl.protocol.task.crdt.pncounter  (1 usage found)
                    PNCounterGetConfiguredReplicaCountMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            73 public String getMethodName() {
                com.hazelcast.client.impl.protocol.task.map  (5 usages found)
                    MapFetchNearCacheInvalidationMetadataTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            78 public String getMethodName() {
                    MapMadePublishableMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            68 public String getMethodName() {
                    MapPublisherCreateMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            148 public String getMethodName() {
                    MapPublisherCreateWithValueMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            151 public String getMethodName() {
                    MapSetReadCursorMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            67 public String getMethodName() {
                com.hazelcast.client.impl.protocol.task.schema  (3 usages found)
                    FetchSchemaMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            68 public String getMethodName() {
                    SendAllSchemasMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            70 public String getMethodName() {
                    SendSchemaMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            71 public String getMethodName() {
                com.hazelcast.client.impl.protocol.task.transaction  (5 usages found)
                    XAClearRemoteTransactionMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            82 public String getMethodName() {
                    XACollectTransactionsMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            98 public String getMethodName() {
                    XAFinalizeTransactionMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            66 public String getMethodName() {
                    XATransactionCreateMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            70 public String getMethodName() {
                    XATransactionPrepareMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            75 public String getMethodName() {
                com.hazelcast.cp.internal.datastructures.semaphore.client  (1 usage found)
                    GetSemaphoreTypeMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            71 public String getMethodName() {
                com.hazelcast.cp.internal.session.client  (1 usage found)
                    AbstractSessionMessageTask.java  (1 usage found)
                        getMethodName()  (1 usage found)
                            62 public String getMethodName() {

hz-devops-test · 2023-07-14T16:18:49Z

The job Hazelcast-pr-builder of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file

--------------------------
-------TEST FAILURE-------
--------------------------
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   StreamKafkaPTest.when_processingGuaranteeNoneWithConsumerGroup_then_continueFromLastReadMessageAfterJobRestart:267->testWithJobRestart:350->HazelcastTestSupport.assertTrueEventually:1184->HazelcastTestSupport.assertTrueEventually:1165->lambda$testWithJobRestart$4:350 expected:<200> but was:<193>
[INFO] 
[ERROR] Tests run: 51, Failures: 1, Errors: 0, Skipped: 1
[INFO] 
[ERROR] There are test failures.

...rc/main/java/com/hazelcast/client/impl/protocol/task/cache/CacheCreateConfigMessageTask.java

...java/com/hazelcast/client/impl/protocol/task/cache/CacheListenerRegistrationMessageTask.java

...ast/client/impl/protocol/task/executorservice/ExecutorServiceSubmitToAddressMessageTask.java

...t/client/impl/protocol/task/executorservice/ExecutorServiceSubmitToPartitionMessageTask.java

...st/src/main/java/com/hazelcast/client/impl/protocol/task/map/MapDestroyCacheMessageTask.java

...n/java/com/hazelcast/client/impl/protocol/task/transaction/TransactionCommitMessageTask.java

...n/java/com/hazelcast/client/impl/protocol/task/transaction/TransactionCreateMessageTask.java

...java/com/hazelcast/client/impl/protocol/task/transaction/TransactionRollbackMessageTask.java

...java/com/hazelcast/client/impl/protocol/task/transaction/XATransactionCommitMessageTask.java

...va/com/hazelcast/client/impl/protocol/task/transaction/XATransactionRollbackMessageTask.java

srknzl

lgtm except I think we should make sure we document these if the user will need to use these strings. According to https://docs.hazelcast.com/imdg/4.2/security/security-interceptor#latest-banner, the user needs to know the exact method name.

If they need to know the exact strings, we should list these strings in our documentation, together with the methods each string corresponds to. e.g "destroyCache" -> "QueryCache.destroy" and "IMap.destroy"

Another question: Can we use the same strings (e.g using "create" for two different message tasks) in two different getMethodName's? What happens in that case ? If it is not allowed, do we validate if we are not duplicating any string in getMethodName?

ihsandemir · 2023-07-18T14:05:34Z

@srknzl The unique one is service name + method name, hence, it is ok to have the same method name e.g. using "create" for two different message tasks since their service names will differ.

kwart

Could you place the names in a separate constant class to make it a public API?

I think we need to cover more classes - the comment mentions CreateProxyMessageTask and DestroyProxyMessageTask.

Also these ones from your list caught my eye:

                    DeployClassesMessageTask.java  (1 usage found)
                    CacheDestroyMessageTask.java  (1 usage found)

We could also consider the ones with *Config* in the name.

ihsandemir · 2023-07-18T16:03:14Z

Could you place the names in a separate constant class to make it a public API?

I think we need to cover more classes - the comment mentions CreateProxyMessageTask and DestroyProxyMessageTask.

Also these ones from your list caught my eye:
                    DeployClassesMessageTask.java  (1 usage found)
                    CacheDestroyMessageTask.java  (1 usage found)
We could also consider the ones with *Config* in the name.

I will put them in a new constants file com.hazelcast.security.SecurityInterceptorConstants as requested.

@kwart What about the objectType (service names)? Should we also have them among the constants? The user needs them.

Can the user do security check on proxy creation which happens for each data structure?

ihsandemir · 2023-07-27T14:03:44Z

Could you place the names in a separate constant class to make it a public API?

I think we need to cover more classes - the comment mentions CreateProxyMessageTask and DestroyProxyMessageTask.

Also these ones from your list caught my eye:
                    DeployClassesMessageTask.java  (1 usage found)
                    CacheDestroyMessageTask.java  (1 usage found)
We could also consider the ones with *Config* in the name.

@kwart @srknzl What about using enums instead of free String in the parameter.

diff --git a/hazelcast/src/main/java/com/hazelcast/security/SecurityInterceptor.java b/hazelcast/src/main/java/com/hazelcast/security/SecurityInterceptor.java
index 65a583a409..bf6cca5198 100644
--- a/hazelcast/src/main/java/com/hazelcast/security/SecurityInterceptor.java
+++ b/hazelcast/src/main/java/com/hazelcast/security/SecurityInterceptor.java
@@ -23,6 +23,20 @@ import java.security.AccessControlException;
  */
 public interface SecurityInterceptor {
+    public enum ObjectName {
+        PUT(“put”);
+
+        private final String methodName;
+
+        ObjectName(String methodName) {
+            this.methodName = methodName;
+        }
+
+        public String getMethodName() {
+            return methodName;
+        }
+    }
+
     /**
      *
      * @param credentials
@@ -43,5 +57,5 @@ public interface SecurityInterceptor {
      * @param methodName
      * @param parameters
      */
-    void after(Credentials credentials, String objectType, String objectName, String methodName, Parameters parameters);
+    void after(Credentials credentials, String objectType, ObjectName objectName, String methodName, Parameters parameters);
 }

Is there any specific reason to make the parameter as free String?
We deprecate the old one and put the new one in place. what do you think?

kwart · 2023-07-31T12:00:08Z

As discussed internally, the enums (compared to free Strings) can lead to NoSuchFieldError thrown. I.e.

for backward compatibility you wouldn't be allowed to remove a value/constant from enum in newer Hazelcast versions;
interceptor developed for one version might not work with older Hazelcast versions.

hz-devops-test · 2023-07-31T15:42:35Z

The job Hazelcast-pr-builder of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file

---------ERRORS-----------
--------------------------
[ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-builder/hazelcast/src/main/java/com/hazelcast/security/SecurityInterceptorConstants.java:19:1: Utility classes should not have a public or default constructor. [HideUtilityClassConstructor]
--------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.3.0:checkstyle (default) on project hazelcast: An error has occurred in Checkstyle report generation. Failed during checkstyle execution: There is 1 error reported by Checkstyle 8.38 with /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-builder/checkstyle/checkstyle.xml ruleset. -> [Help 1]
--------------------------
[ERROR] 
--------------------------
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
--------------------------
[ERROR] 
--------------------------
[ERROR] For more information about the errors and possible solutions, please read the following articles:
--------------------------
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
--------------------------
[ERROR] 
--------------------------
[ERROR] After correcting the problems, you can resume the build with the command
--------------------------
[ERROR]   mvn  -rf :hazelcast
--------------------------

hz-devops-test · 2023-07-31T15:45:03Z

The job Hazelcast-pr-compiler of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file

---------ERRORS-----------
--------------------------
[ERROR] /appdisk/jenkins/jenkins_slave/workspace/Hazelcast-pr-compiler/hazelcast/src/main/java/com/hazelcast/security/SecurityInterceptorConstants.java:19:1: Utility classes should not have a public or default constructor. [HideUtilityClassConstructor]
--------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.3.0:checkstyle (default) on project hazelcast: An error has occurred in Checkstyle report generation. Failed during checkstyle execution: There is 1 error reported by Checkstyle 8.38 with /appdisk/jenkins/jenkins_slave/workspace/Hazelcast-pr-compiler/checkstyle/checkstyle.xml ruleset. -> [Help 1]
--------------------------
[ERROR] 
--------------------------
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
--------------------------
[ERROR] 
--------------------------
[ERROR] For more information about the errors and possible solutions, please read the following articles:
--------------------------
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
--------------------------
[ERROR] 
--------------------------
[ERROR] After correcting the problems, you can resume the build with the command
--------------------------
[ERROR]   mvn  -rf :hazelcast
--------------------------

srknzl · 2023-07-31T19:16:46Z

...m/hazelcast/client/impl/protocol/task/cache/CacheRemovePartitionLostListenerMessageTask.java

@@ -75,7 +76,7 @@ public Permission getRequiredPermission() {

    @Override
    public String getMethodName() {
-        return "removeCachePartitionLostListener";
+        return SecurityInterceptorConstants.REMOVE_PARTITION_LOST_LISTENER;


The string is changed as I see: removePartitionLostListener. Is it intentional ?

Yes, it is intentional. There is the getServiceName method as well at the Security interceptor which identifies if it is cache service or not, hence no need for the work Cache in the method name and I removed it.

srknzl · 2023-07-31T19:19:23Z

...ast/src/main/java/com/hazelcast/client/impl/protocol/task/cache/CacheIterateMessageTask.java

@@ -77,6 +78,6 @@ public Object[] getParameters() {

    @Override
    public String getMethodName() {
-        return "iterator";
+        return SecurityInterceptorConstants.FETCH;


Should not this be SecurityInterceptorConstants.ITERATE? 🤔

They actually correspond to the hasNext and next methods of the iterator. I am changing it back to iterator as suggested. fix commit

hz-devops-test · 2023-08-09T19:04:09Z

The job Hazelcast-pr-builder of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file

---------ERRORS-----------
--------------------------
[ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-builder/hazelcast/src/main/java/com/hazelcast/security/SecurityInterceptorConstants.java:24:1: Utility classes should not have a public or default constructor. [HideUtilityClassConstructor]
--------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.3.0:checkstyle (default) on project hazelcast: An error has occurred in Checkstyle report generation. Failed during checkstyle execution: There is 1 error reported by Checkstyle 8.38 with /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-builder/checkstyle/checkstyle.xml ruleset. -> [Help 1]
--------------------------
[ERROR] 
--------------------------
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
--------------------------
[ERROR] 
--------------------------
[ERROR] For more information about the errors and possible solutions, please read the following articles:
--------------------------
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
--------------------------
[ERROR] 
--------------------------
[ERROR] After correcting the problems, you can resume the build with the command
--------------------------
[ERROR]   mvn  -rf :hazelcast
--------------------------

hz-devops-test · 2023-08-09T19:05:13Z

The job Hazelcast-pr-EE-compiler of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file

--------------------------
---------SUMMARY----------
--------------------------
[ERROR] COMPILATION ERROR : 
--------------------------
[ERROR] /appdisk/jenkins/jenkins_slave/workspace/Hazelcast-pr-EE-compiler/hazelcast/hazelcast/src/main/java/com/hazelcast/client/impl/protocol/task/map/MapAddEntryListenerMessageTask.java:[30,8] com.hazelcast.client.impl.protocol.task.map.MapAddEntryListenerMessageTask is not abstract and does not override abstract method getMethodName() in com.hazelcast.client.impl.protocol.task.AbstractMessageTask
--------------------------
---------ERRORS-----------
--------------------------
[ERROR] COMPILATION ERROR : 
--------------------------
[ERROR] /appdisk/jenkins/jenkins_slave/workspace/Hazelcast-pr-EE-compiler/hazelcast/hazelcast/src/main/java/com/hazelcast/client/impl/protocol/task/map/MapAddEntryListenerMessageTask.java:[30,8] com.hazelcast.client.impl.protocol.task.map.MapAddEntryListenerMessageTask is not abstract and does not override abstract method getMethodName() in com.hazelcast.client.impl.protocol.task.AbstractMessageTask
--------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:compile (default-compile) on project hazelcast: Compilation failure
--------------------------
[ERROR] /appdisk/jenkins/jenkins_slave/workspace/Hazelcast-pr-EE-compiler/hazelcast/hazelcast/src/main/java/com/hazelcast/client/impl/protocol/task/map/MapAddEntryListenerMessageTask.java:[30,8] com.hazelcast.client.impl.protocol.task.map.MapAddEntryListenerMessageTask is not abstract and does not override abstract method getMethodName() in com.hazelcast.client.impl.protocol.task.AbstractMessageTask
--------------------------
[ERROR] -> [Help 1]
--------------------------
[ERROR] 
--------------------------
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
--------------------------
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
--------------------------
[ERROR] 
--------------------------
[ERROR] For more information about the errors and possible solutions, please read the following articles:
--------------------------
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
--------------------------
[ERROR] 
--------------------------
[ERROR] After correcting the problems, you can resume the build with the command
--------------------------
[ERROR]   mvn  -rf :hazelcast
--------------------------

hz-devops-test · 2023-08-09T19:06:28Z

The job Hazelcast-pr-compiler of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file

---------ERRORS-----------
--------------------------
[ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-compiler/hazelcast/src/main/java/com/hazelcast/security/SecurityInterceptorConstants.java:24:1: Utility classes should not have a public or default constructor. [HideUtilityClassConstructor]
--------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.3.0:checkstyle (default) on project hazelcast: An error has occurred in Checkstyle report generation. Failed during checkstyle execution: There is 1 error reported by Checkstyle 8.38 with /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-compiler/checkstyle/checkstyle.xml ruleset. -> [Help 1]
--------------------------
[ERROR] 
--------------------------
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
--------------------------
[ERROR] 
--------------------------
[ERROR] For more information about the errors and possible solutions, please read the following articles:
--------------------------
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
--------------------------
[ERROR] 
--------------------------
[ERROR] After correcting the problems, you can resume the build with the command
--------------------------
[ERROR]   mvn  -rf :hazelcast
--------------------------

ihsandemir · 2023-09-13T09:41:30Z

I am not putting any security checks on the compact schema management tasks at package com.hazelcast.client.impl.protocol.task.schema;. These are for internal usage.

ihsandemir · 2023-09-13T11:18:48Z

@kwart @srknzl I updated the Pr based on comments from Josef. Can you guys review the latest?

srknzl · 2023-09-13T12:15:22Z

...va/com/hazelcast/client/impl/protocol/task/transaction/XACollectTransactionsMessageTask.java

@@ -96,7 +97,7 @@ public String getDistributedObjectName() {

    @Override
    public String getMethodName() {
-        return null;
+        return SecurityInterceptorConstants.RECOVER;


collect transactions and recover? What is the relation ?

When I trace this task and try to find the associated user API, I found that the user API is XAResource.recover API and hence I named it it this way to relate to the user API.

srknzl

approving with one comment above

kwart · 2023-09-15T15:39:28Z

run-ee-tests

kwart · 2023-09-15T15:39:36Z

run-ee-nightly-tests

hz-devops-test · 2023-09-15T19:06:32Z

The job Hazelcast-pr-builder-ee-tests of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file

---------ERRORS-----------
--------------------------
[ERROR] Tests run: 13, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 20.14 s <<< FAILURE! -- in com.hazelcast.client.security.SetSecurityInterceptorTest
--------------------------
[ERROR] com.hazelcast.client.security.SetSecurityInterceptorTest.iterator -- Time elapsed: 20.13 s <<< FAILURE!
--------------------------
[ERROR] Tests run: 29, Failures: 1, Errors: 0, Skipped: 10, Time elapsed: 20.22 s <<< FAILURE! -- in com.hazelcast.client.security.CacheSecurityInterceptorTest
--------------------------
[ERROR] com.hazelcast.client.security.CacheSecurityInterceptorTest.removeAll -- Time elapsed: 20.20 s <<< FAILURE!
--------------------------
[ERROR] Tests run: 5, Failures: 0, Errors: 2, Skipped: 0, Time elapsed: 843.0 s <<< FAILURE! -- in com.hazelcast.internal.tstore.compaction.PartitionCompactorTest
--------------------------
[ERROR] com.hazelcast.internal.tstore.compaction.PartitionCompactorTest.test_partition_compactor_with_index_on_owner_and_backup -- Time elapsed: 302.3 s <<< ERROR!
--------------------------
[ERROR] com.hazelcast.internal.tstore.compaction.PartitionCompactorTest.test_partition_compactor_with_null_index_on_owner_and_backup -- Time elapsed: 300.8 s <<< ERROR!
--------------------------
[ERROR] Failures: 
--------------------------
[ERROR]   CacheSecurityInterceptorTest.removeAll Expected: removeAll, Actual: removeAllKeys
--------------------------
[ERROR]   SetSecurityInterceptorTest.iterator Expected: iterator, Actual: getAll
--------------------------
[ERROR] Errors: 
--------------------------
[ERROR]   PartitionCompactorTest.test_partition_compactor_with_index_on_owner_and_backup:149 ? TestTimedOut test timed out after 300000 milliseconds
--------------------------
[ERROR]   PartitionCompactorTest.test_partition_compactor_with_null_index_on_owner_and_backup:205 ? TestTimedOut test timed out after 300000 milliseconds
--------------------------
[ERROR] Tests run: 10183, Failures: 2, Errors: 2, Skipped: 78
--------------------------
[ERROR] There are test failures.
--------------------------
--------------------------
-------TEST FAILURE-------
--------------------------
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   CacheSecurityInterceptorTest.removeAll Expected: removeAll, Actual: removeAllKeys
[ERROR]   SetSecurityInterceptorTest.iterator Expected: iterator, Actual: getAll
[ERROR] Errors: 
[ERROR]   PartitionCompactorTest.test_partition_compactor_with_index_on_owner_and_backup:149 ? TestTimedOut test timed out after 300000 milliseconds
[ERROR]   PartitionCompactorTest.test_partition_compactor_with_null_index_on_owner_and_backup:205 ? TestTimedOut test timed out after 300000 milliseconds
[INFO] 
[ERROR] Tests run: 10183, Failures: 2, Errors: 2, Skipped: 78
[INFO] 
[ERROR] There are test failures.

kwart

Could you also send a fix for the 2 EE tests that fail with the new names?

If we proceed with the rename (i.e. change from a not-null value to another one), then these instances should be listed in the Release Notes as breaking changes. We should make doc and support teams aware of it.

Overall, I like the changes. The majority of my comments are just process ones :)

kwart · 2023-09-18T07:07:38Z

...cast/src/main/java/com/hazelcast/client/impl/protocol/task/cache/CachePutAllMessageTask.java

@@ -70,14 +75,24 @@ public String getDistributedObjectName() {

    @Override
    public Object[] getParameters() {
+        Map<Data, Data> map = createMap();


Does it make sense to put the parameters on a new map? The current implementation will create the Map every time once the security is enabled - even if there is no SecurityInterceptor configured.

This is to make the interceptor test at here work properly and it makes sense since the parameter of the ICache.putAll method receives a map. Yes, it is one more map creation but it makes the API more user friendly. Let me know if it is OK or I am happy to revert it if we agree on returning the parameters.entries

public java.util.List<java.util.Map.Entry<com.hazelcast.internal.serialization.Data, com.hazelcast.internal.serialization.Data>> entries;

hazelcast/src/main/java/com/hazelcast/security/SecurityInterceptorConstants.java

kwart · 2023-09-18T07:45:02Z

...lcast/src/main/java/com/hazelcast/client/impl/protocol/task/map/MapFetchKeysMessageTask.java

@@ -77,7 +78,7 @@ public String getDistributedObjectName() {

    @Override
    public String getMethodName() {
-        return "iterator";
+        return SecurityInterceptorConstants.ITERATOR_FETCH_KEYS;


This should go to the Release Notes.

kwart · 2023-09-18T08:19:46Z

.../src/main/java/com/hazelcast/client/impl/protocol/task/map/MapFetchWithQueryMessageTask.java

@@ -105,7 +106,7 @@ public String getDistributedObjectName() {

    @Override
    public String getMethodName() {
-        return "iterator";
+        return SecurityInterceptorConstants.ITERATOR_FETCH_WITH_QUERY;


kwart · 2023-09-18T08:21:16Z

.../java/com/hazelcast/client/impl/protocol/task/map/MapProjectionWithPredicateMessageTask.java

@@ -67,7 +68,7 @@ public String getDistributedObjectName() {

    @Override
    public String getMethodName() {
-        return "projectWithPredicate";
+        return SecurityInterceptorConstants.PROJECT;


kwart · 2023-09-18T09:05:45Z

.../protocol/task/scheduledexecutor/ScheduledExecutorTaskGetResultFromPartitionMessageTask.java

@@ -76,7 +77,7 @@ public String getDistributedObjectName() {

    @Override
    public String getMethodName() {
-        return "getResultTimeout";
+        return SecurityInterceptorConstants.GET_RESULT;


kwart · 2023-09-18T09:10:27Z

...mpl/protocol/task/scheduledexecutor/ScheduledExecutorTaskGetResultFromTargetMessageTask.java

@@ -82,7 +83,7 @@ public String getDistributedObjectName() {

    @Override
    public String getMethodName() {
-        return "getResultTimeout";
+        return SecurityInterceptorConstants.GET_RESULT;


kwart · 2023-09-18T09:13:09Z

hazelcast/src/main/java/com/hazelcast/client/impl/protocol/task/set/SetGetAllMessageTask.java

@@ -72,7 +73,7 @@ public Permission getRequiredPermission() {

    @Override
    public String getMethodName() {
-        return "iterator";
+        return SecurityInterceptorConstants.GET_ALL;


kwart · 2023-09-18T10:16:12Z

...m/hazelcast/client/impl/protocol/task/transactionallist/TransactionalListAddMessageTask.java

@@ -75,7 +76,7 @@ public String getDistributedObjectName() {

    @Override
    public String getMethodName() {
-        return "set";
+        return SecurityInterceptorConstants.ADD;


kwart · 2023-09-18T10:19:55Z

...nt/impl/protocol/task/transactionalmultimap/TransactionalMultiMapRemoveEntryMessageTask.java

@@ -75,7 +76,7 @@ public String getDistributedObjectName() {

    @Override
    public String getMethodName() {
-        return "removeEntry";
+        return SecurityInterceptorConstants.REMOVE;


…d non-null method names to some of the message tasks which were missing the names and can be used in security interceptor since they are related to the user method calls. The ones which return null for the getMethodName function are the internally used or not directly related to the user API calls. Also, removed the default null returning implementations in some of the base message tasks.

…terceptor is being called. Removed the name from the CacheCreateConfigMessageTask since no such interception is provided for all other structures. Fixed the returned parameters in loadAll, and putAll message tasks and opened the previously ignored tests.

…stants class `SecurityInterceptorConstants`.

kwart

Thanks for the enhancements, Ihsan. 👍

hz-devops-test · 2023-09-26T08:54:42Z

The job Hazelcast-pr-builder of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file

---------ERRORS-----------
--------------------------
[ERROR] Tests run: 13, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 3.172 s <<< FAILURE! -- in com.hazelcast.internal.tpcengine.nio.NioAsyncServerSocketTest
--------------------------
[ERROR] com.hazelcast.internal.tpcengine.nio.NioAsyncServerSocketTest.test_createCloseLoop_withSameReactor -- Time elapsed: 0.126 s <<< ERROR!
--------------------------
[ERROR] Errors: 
--------------------------
[ERROR]   NioAsyncServerSocketTest>AsyncServerSocketTest.test_createCloseLoop_withSameReactor:255 ? UncheckedIO Failed to bind to /127.0.0.1:5001
--------------------------
[ERROR] Tests run: 292, Failures: 0, Errors: 1, Skipped: 0
--------------------------
[ERROR] 
--------------------------

ihsandemir · 2023-09-26T11:24:59Z

run-lab-run

…me (#25551) Fixed the incorrectly changed method names at [merged PR](#25020) back to the original name. Fixes the test failure at [this](hazelcast/hazelcast-enterprise#6548) PR [run](https://jenkins.hazelcast.com/job/Hazelcast-EE-pr-builder/9256/#showFailuresLink).

…037] (hazelcast#25020) Return meaningful values for MessageTask `getMethodName` function. Added non-null method names to some of the message tasks which were missing the names and can be used in security interceptor since they are related to the user method calls. The ones which return null for the `getMethodName` function are the internally used or not directly related to the user API calls. Also, removed the default null returning implementations in some of the base message tasks. The changes at the method names should be mentioned at the Release Notes. !!! EE PR: hazelcast/hazelcast-enterprise#6252

…me (hazelcast#25551) Fixed the incorrectly changed method names at [merged PR](hazelcast#25020) back to the original name. Fixes the test failure at [this](hazelcast/hazelcast-enterprise#6548) PR [run](https://jenkins.hazelcast.com/job/Hazelcast-EE-pr-builder/9256/#showFailuresLink).

…037] (hazelcast#25020) Return meaningful values for MessageTask `getMethodName` function. Added non-null method names to some of the message tasks which were missing the names and can be used in security interceptor since they are related to the user method calls. The ones which return null for the `getMethodName` function are the internally used or not directly related to the user API calls. Also, removed the default null returning implementations in some of the base message tasks. The changes at the method names should be mentioned at the Release Notes. !!! EE PR: hazelcast/hazelcast-enterprise#6252

ihsandemir added the Type: Defect label Jul 14, 2023

ihsandemir added this to the 5.4.0 milestone Jul 14, 2023

ihsandemir requested review from kwart and srknzl July 14, 2023 15:25

ihsandemir requested a review from a team as a code owner July 14, 2023 15:25

ihsandemir self-assigned this Jul 14, 2023

srknzl reviewed Jul 14, 2023

View reviewed changes

srknzl approved these changes Jul 17, 2023

View reviewed changes

kwart requested changes Jul 18, 2023

View reviewed changes

srknzl approved these changes Jul 31, 2023

View reviewed changes

srknzl reviewed Jul 31, 2023

View reviewed changes

ihsandemir force-pushed the messageTaskMethodNames branch from fb29146 to 28cb19f Compare August 10, 2023 15:10

ihsandemir added a commit to ihsandemir/hazelcast that referenced this pull request Sep 13, 2023

Review comment fix for comment hazelcast#25020 (comment)

d743c62

srknzl reviewed Sep 13, 2023

View reviewed changes

srknzl approved these changes Sep 13, 2023

View reviewed changes

kwart self-requested a review September 15, 2023 15:40

kwart requested changes Sep 18, 2023

View reviewed changes

ihsandemir added 9 commits September 26, 2023 10:44

review comment fixes.

df3b1c9

Review comment fix: Put all method name constants into the public con…

527f3cd

…stants class `SecurityInterceptorConstants`.

Review comment fixes.

a578730

more constants.

b7d58f7

Added more constants.

d041660

Added more constants.

92d632e

Review comment fix for comment hazelcast#25020 (comment)

0c8def3

ihsandemir force-pushed the messageTaskMethodNames branch from d743c62 to 0c8def3 Compare September 26, 2023 07:52

kwart approved these changes Sep 26, 2023

View reviewed changes

ihsandemir added the Add to Release Notes label Sep 26, 2023

ihsandemir merged commit b17cd21 into hazelcast:master Sep 26, 2023
8 checks passed

ihsandemir deleted the messageTaskMethodNames branch September 26, 2023 14:38

ihsandemir mentioned this pull request Sep 27, 2023

Reverted the incorrectly changed method names back to the original name #25551

Merged

ihsandemir mentioned this pull request Oct 10, 2023

Drafting the RN for Platform 5.4.0. hazelcast/hz-docs#792

Closed

Return meaningful values for MessageTask getMethodName function[API-2037] #25020

Return meaningful values for MessageTask getMethodName function[API-2037] #25020

Conversation

ihsandemir commented Jul 14, 2023 • edited

ihsandemir commented Jul 14, 2023

hz-devops-test commented Jul 14, 2023

srknzl left a comment • edited

Choose a reason for hiding this comment

ihsandemir commented Jul 18, 2023

kwart left a comment

Choose a reason for hiding this comment

ihsandemir commented Jul 18, 2023 • edited

ihsandemir commented Jul 27, 2023

kwart commented Jul 31, 2023

hz-devops-test commented Jul 31, 2023

hz-devops-test commented Jul 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihsandemir Sep 13, 2023 • edited

Choose a reason for hiding this comment

hz-devops-test commented Aug 9, 2023

hz-devops-test commented Aug 9, 2023

hz-devops-test commented Aug 9, 2023

ihsandemir commented Sep 13, 2023

ihsandemir commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srknzl left a comment

Choose a reason for hiding this comment

kwart commented Sep 15, 2023

kwart commented Sep 15, 2023

hz-devops-test commented Sep 15, 2023

kwart left a comment • 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

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

kwart left a comment

Choose a reason for hiding this comment

hz-devops-test commented Sep 26, 2023

ihsandemir commented Sep 26, 2023

ihsandemir commented Jul 14, 2023 •

edited

srknzl left a comment •

edited

ihsandemir commented Jul 18, 2023 •

edited

ihsandemir Sep 13, 2023 •

edited

kwart left a comment •

edited