[GitHub] [shardingsphere] tristaZero commented on a change in pull request #10310: Users removed from StandardMetaDataContexts

[GitBox <gi...@apache.org>]

tristaZero commented on a change in pull request #10310:
URL: https://github.com/apache/shardingsphere/pull/10310#discussion_r630955100



##########
File path: shardingsphere-proxy/shardingsphere-proxy-frontend/shardingsphere-proxy-frontend-mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/authentication/MySQLAuthenticationHandler.java
##########
@@ -53,14 +57,24 @@
      * @return login success or failure
      */
     public Optional<MySQLServerErrorCode> login(final String username, final String hostname, final byte[] authenticationResponse, final String databaseName) {
-        Optional<ShardingSphereUser> user = ProxyContext.getInstance().getMetaDataContexts().getUsers().findUser(new Grantee(username, hostname));
+        Optional<ShardingSphereUser> user = getUsersFromGlobalRuleMetaData().findUser(new Grantee(username, hostname));
         if (!user.isPresent() || !isPasswordRight(user.get().getPassword(), authenticationResponse)) {
             return Optional.of(MySQLServerErrorCode.ER_ACCESS_DENIED_ERROR);
         }
         return null == databaseName || SQLCheckEngine.check(databaseName, getRules(databaseName), user.get().getGrantee())
                 ? Optional.empty() : Optional.of(MySQLServerErrorCode.ER_DBACCESS_DENIED_ERROR);
     }
-    
+
+    private ShardingSphereUsers getUsersFromGlobalRuleMetaData() {
+        for (RuleConfiguration ruleConfig : ProxyContext.getInstance().getMetaDataContexts().getGlobalRuleMetaData().getConfigurations()) {
+            if (ruleConfig instanceof AuthorityRuleConfiguration) {
+                AuthorityRuleConfiguration authorityRuleConfiguration = (AuthorityRuleConfiguration) ruleConfig;
+                return new ShardingSphereUsers(authorityRuleConfiguration.getUsers());
+            }
+        }
+        return new ShardingSphereUsers(Collections.emptyList());
+    }

Review comment:
       Hi,
   I guess you still have this sense that this approach is not so elegant, don't you? Here an idea FYI,
   We can add a new interface `check(Grantee)` in `SQLCheckEngine`, `SQLChecker` and its implementation, i.e., `AuthorityChecker`. Consequently, all authority checking processes will be hidden in `AuthorityChecker`.
   How do you think?

##########
File path: shardingsphere-governance/shardingsphere-governance-context/src/main/java/org/apache/shardingsphere/governance/context/metadata/GovernanceMetaDataContexts.java
##########
@@ -225,10 +217,8 @@ public synchronized void renew(final PropertiesChangedEvent event) {
      */
     @Subscribe
     public synchronized void renew(final AuthorityChangedEvent event) {
-        Collection<ShardingSphereUser> users = new HashSet<>(getNewUsers(event.getUsers()));
-        users.addAll(getModifiedUsers(event.getUsers()));
         metaDataContexts = new StandardMetaDataContexts(
-                metaDataContexts.getMetaDataMap(), metaDataContexts.getGlobalRuleMetaData(), metaDataContexts.getExecutorEngine(), new ShardingSphereUsers(users), metaDataContexts.getProps());
+                metaDataContexts.getMetaDataMap(), metaDataContexts.getGlobalRuleMetaData(), metaDataContexts.getExecutorEngine(), metaDataContexts.getProps());

Review comment:
       It is pointless to assign a copied `StandardMetaDataContexts` to `metaDataContexts`.
   getChangedMetaData() of `renew(final RuleConfigurationsChangedEvent event)` is a better method to create a new `global authority rule` to truly `renew`. (Not efficient, but correct)

##########
File path: shardingsphere-governance/shardingsphere-governance-context/src/test/java/org/apache/shardingsphere/governance/context/metadata/GovernanceMetaDataContextsTest.java
##########
@@ -155,12 +154,12 @@ public void assertPropertiesChanged() {
         governanceMetaDataContexts.renew(event);
         assertThat(governanceMetaDataContexts.getProps().getProps().getProperty(ConfigurationPropertyKey.SQL_SHOW.getKey()), is("true"));
     }
-    
+
     @Test
     public void assertAuthorityChanged() {
         AuthorityChangedEvent event = new AuthorityChangedEvent(Collections.singleton(mock(ShardingSphereUser.class)));
         governanceMetaDataContexts.renew(event);
-        assertThat(governanceMetaDataContexts.getUsers().getUsers().size(), is(1));
+        //assertThat(governanceMetaDataContexts.getUsers().getUsers().size(), is(1));

Review comment:
       So why is this statement commented?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org