Skip to content

Commit

Permalink
Fix flaky-test: AuthorizationTest.simple (apache#9740)
Browse files Browse the repository at this point in the history
Fixes apache#9709

### Motivation
```
Caused by: java.lang.NullPointerException
	at java.util.TreeMap.getEntry(TreeMap.java:347)
```
From the stack trace, it is caused by the key read by `TreeMap` being null. Therefore, the role must be null.
```
//PulsarAuthorizationProvider.java:404
Map<String, Set<AuthAction>> namespaceRoles = policies.get().auth_policies.namespace_auth;
Set<AuthAction> namespaceActions = namespaceRoles.get(role);
```

According to the code, the role is read through `httpRequest.getAttribute` in `PulsarWebResource#clientAppId`, so it is normal to be null.

Although the role is null, why do unit tests pass occasionally?
I added logs and found that the implementation class of Map is not always TreeMap, but occasionally HashMap.
When it is a HashMap, no error will be reported if the key is null. This problem is caused by Jackson's serialization.

I checked AuthPolicies, all Maps are initialized as TreeMap, but we have never used any features of TreeMap. 
In order to avoid serialization problems elsewhere, I changed TreeMap to HashMap.
Another way is to add the annotation @JsonDeserialize(as = TreeMap.class), and then make the judgment that the key is null.
Why was TreeMap used here? May need @merlimat  to help explain.
  • Loading branch information
315157973 authored Mar 4, 2021
1 parent 36b44c3 commit b2796f3
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,11 @@
import io.netty.buffer.ByteBuf;
import java.io.IOException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.TreeMap;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionException;
import java.util.concurrent.ExecutionException;
Expand Down Expand Up @@ -198,7 +198,7 @@ protected Map<String, Set<AuthAction>> internalGetPermissionsOnTopic() {
Policies policies = namespaceResources().get(path(POLICIES, namespaceName.toString()))
.orElseThrow(() -> new RestException(Status.NOT_FOUND, "Namespace does not exist"));

Map<String, Set<AuthAction>> permissions = Maps.newTreeMap();
Map<String, Set<AuthAction>> permissions = Maps.newHashMap();
AuthPolicies auth = policies.auth_policies;

// First add namespace level permissions
Expand Down Expand Up @@ -323,7 +323,7 @@ private void grantPermissions(String topicUri, String role, Set<AuthAction> acti
try {
namespaceResources().set(path(POLICIES, namespaceName.toString()), (policies) -> {
if (!policies.auth_policies.destination_auth.containsKey(topicUri)) {
policies.auth_policies.destination_auth.put(topicUri, new TreeMap<String, Set<AuthAction>>());
policies.auth_policies.destination_auth.put(topicUri, new HashMap<>());
}

policies.auth_policies.destination_auth.get(topicUri).put(role, actions);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ public class AuthPolicies {
public final Map<String, Set<String>> subscription_auth_roles;

public AuthPolicies() {
namespace_auth = Maps.newTreeMap();
destination_auth = Maps.newTreeMap();
subscription_auth_roles = Maps.newTreeMap();
namespace_auth = Maps.newHashMap();
destination_auth = Maps.newHashMap();
subscription_auth_roles = Maps.newHashMap();
}

@Override
Expand Down

0 comments on commit b2796f3

Please sign in to comment.