Tidy up some DLS cache code (#132416)

Author: joegallo

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCache.java

@@ -55,32 +55,34 @@
 /**
  * This is a cache for {@link BitSet} instances that are used with the {@link DocumentSubsetReader}.
  * It is bounded by memory size and access time.
- *
+ * <p>
  * DLS uses {@link BitSet} instances to track which documents should be visible to the user ("live") and which should not ("dead").
  * This means that there is a bit for each document in a Lucene index (ES shard).
  * Consequently, an index with 10 million document will use more than 1Mb of bitset memory for every unique DLS query, and an index
  * with 1 billion documents will use more than 100Mb of memory per DLS query.
  * Because DLS supports templating queries based on user metadata, there may be many distinct queries in use for each index, even if
  * there is only a single active role.
- *
+ * <p>
  * The primary benefit of the cache is to avoid recalculating the "live docs" (visible documents) when a user performs multiple
  * consecutive queries across one or more large indices. Given the memory examples above, the cache is only useful if it can hold at
  * least 1 large (100Mb or more ) {@code BitSet} during a user's active session, and ideally should be capable of support multiple
  * simultaneous users with distinct DLS queries.
- *
+ * <p>
  * For this reason the default memory usage (weight) for the cache set to 10% of JVM heap ({@link #CACHE_SIZE_SETTING}), so that it
  * automatically scales with the size of the Elasticsearch deployment, and can provide benefit to most use cases without needing
  * customisation. On a 32Gb heap, a 10% cache would be 3.2Gb which is large enough to store BitSets representing 25 billion docs.
- *
+ * <p>
  * However, because queries can be templated by user metadata and that metadata can change frequently, it is common for the
- * effetively lifetime of a single DLS query to be relatively short. We do not want to sacrifice 10% of heap to a cache that is storing
- * BitSets that are not longer needed, so we set the TTL on this cache to be 2 hours ({@link #CACHE_TTL_SETTING}). This time has been
+ * effective lifetime of a single DLS query to be relatively short. We do not want to sacrifice 10% of heap to a cache that is storing
+ * BitSets that are no longer needed, so we set the TTL on this cache to be 2 hours ({@link #CACHE_TTL_SETTING}). This time has been
  * chosen so that it will retain BitSets that are in active use during a user's session, but not be an ongoing drain on memory.
  *
  * @see org.elasticsearch.index.cache.bitset.BitsetFilterCache
  */
 public final class DocumentSubsetBitsetCache implements IndexReader.ClosedListener, Closeable, Accountable {
 
+    private static final Logger logger = LogManager.getLogger(DocumentSubsetBitsetCache.class);
+
     /**
      * The TTL defaults to 2 hours. We default to a large cache size ({@link #CACHE_SIZE_SETTING}), and aggressively
      * expire unused entries so that the cache does not hold on to memory unnecessarily.
@@ -102,8 +104,6 @@ public final class DocumentSubsetBitsetCache implements IndexReader.ClosedListen
 
     private static final BitSet NULL_MARKER = new FixedBitSet(0);
 
-    private static final Logger logger = LogManager.getLogger(DocumentSubsetBitsetCache.class);
-
     /**
      * When a {@link BitSet} is evicted from {@link #bitsetCache}, we need to also remove it from {@link #keysByIndex}.
      * We use a {@link ReentrantReadWriteLock} to control atomicity here - the "read" side represents potential insertions to the
@@ -130,7 +130,8 @@ public DocumentSubsetBitsetCache(Settings settings, ThreadPool threadPool) {
      * @param cleanupExecutor An executor on which the cache cleanup tasks can be run. Due to the way the cache is structured internally,
      *                        it is sometimes necessary to run an asynchronous task to synchronize the internal state.
      */
-    protected DocumentSubsetBitsetCache(Settings settings, ExecutorService cleanupExecutor) {
+    // visible for testing
+    DocumentSubsetBitsetCache(Settings settings, ExecutorService cleanupExecutor) {
         final ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock();
         this.cacheEvictionLock = new ReleasableLock(readWriteLock.writeLock());
         this.cacheModificationLock = new ReleasableLock(readWriteLock.readLock());
@@ -171,7 +172,7 @@ private void onCacheEviction(RemovalNotification<BitsetCacheKey, BitSet> notific
         }
         // We push this to a background thread, so that it reduces the risk of blocking searches, but also so that the lock management is
         // simpler - this callback is likely to take place on a thread that is actively adding something to the cache, and is therefore
-        // holding the read ("update") side of the lock. It is not possible to upgrade a read lock to a write ("eviction") lock, but we
+        // holding the read ("update") side of the lock. It is not possible to upgrade a read lock to a write lock ("eviction"), but we
         // need to acquire that lock here.
         cleanupExecutor.submit(() -> {
             try (ReleasableLock ignored = cacheEvictionLock.acquire()) {
@@ -214,7 +215,7 @@ public long ramBytesUsed() {
     /**
      * Obtain the {@link BitSet} for the given {@code query} in the given {@code context}.
      * If there is a cached entry for that query and context, it will be returned.
-     * Otherwise a new BitSet will be created and stored in the cache.
+     * Otherwise, a new BitSet will be created and stored in the cache.
      * The returned BitSet may be null (e.g. if the query has no results).
      */
     @Nullable
@@ -289,7 +290,7 @@ private static BitSet computeBitSet(Query query, LeafReaderContext context) thro
 
     // Package private for testing
     static boolean isEffectiveMatchAllDocsQuery(Query rewrittenQuery) {
-        if (rewrittenQuery instanceof ConstantScoreQuery && ((ConstantScoreQuery) rewrittenQuery).getQuery() instanceof MatchAllDocsQuery) {
+        if (rewrittenQuery instanceof ConstantScoreQuery csq && csq.getQuery() instanceof MatchAllDocsQuery) {
             return true;
         }
         if (rewrittenQuery instanceof MatchAllDocsQuery) {
@@ -322,7 +323,8 @@ public Map<String, Object> usageStats() {
         return Map.of("count", entryCount(), "memory", ram.toString(), "memory_in_bytes", ram.getBytes());
     }
 
-    private static class BitsetCacheKey {
+    private static final class BitsetCacheKey {
+
         final IndexReader.CacheKey index;
         final Query query;
 

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetReader.java

@@ -195,8 +195,8 @@ public Bits getLiveDocs() {
         computeNumDocsIfNeeded();
         final Bits actualLiveDocs = in.getLiveDocs();
         if (roleQueryBits == null) {
-            // If we would return a <code>null</code> liveDocs then that would mean that no docs are marked as deleted,
-            // but that isn't the case. No docs match with the role query and therefore all docs are marked as deleted
+            // If we were to return a <code>null</code> liveDocs then that would mean that no docs are marked as deleted,
+            // but that isn't the case. No docs match with the role query and therefore all docs are marked as deleted.
             return new Bits.MatchNoBits(in.maxDoc());
         } else if (roleQueryBits instanceof MatchAllBitSet) {
             return actualLiveDocs;

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java

@@ -106,7 +106,7 @@ public boolean hasStoredScript() throws IOException {
 
     /**
      * Creates a {@link BooleanQuery} to be used as filter to restrict access to documents.<br>
-     * Document permission queries are used to create an boolean query.<br>
+     * Document permission queries are used to create a boolean query.<br>
      * If the document permissions are limited, then there is an additional filter added restricting access to documents only allowed by the
      * limited queries.
      *

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/DocumentSubsetBitsetCacheTests.java

@@ -54,6 +54,7 @@
 import java.util.Collections;
 import java.util.IdentityHashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutorService;
@@ -62,8 +63,6 @@
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 
-import static java.util.Collections.emptyList;
-import static java.util.Collections.emptyMap;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
@@ -525,26 +524,13 @@ private void runTestOnIndex(CheckedBiConsumer<SearchExecutionContext, LeafReader
         });
     }
 
-    private static final class TestIndexContext implements Closeable {
-        private final Directory directory;
-        private final IndexWriter indexWriter;
-        private final DirectoryReader directoryReader;
-        private final SearchExecutionContext searchExecutionContext;
-        private final LeafReaderContext leafReaderContext;
-
-        private TestIndexContext(
-            Directory directory,
-            IndexWriter indexWriter,
-            DirectoryReader directoryReader,
-            SearchExecutionContext searchExecutionContext,
-            LeafReaderContext leafReaderContext
-        ) {
-            this.directory = directory;
-            this.indexWriter = indexWriter;
-            this.directoryReader = directoryReader;
-            this.searchExecutionContext = searchExecutionContext;
-            this.leafReaderContext = leafReaderContext;
-        }
+    private record TestIndexContext(
+        Directory directory,
+        IndexWriter indexWriter,
+        DirectoryReader directoryReader,
+        SearchExecutionContext searchExecutionContext,
+        LeafReaderContext leafReaderContext
+    ) implements Closeable {
 
         @Override
         public void close() throws IOException {
@@ -600,7 +586,7 @@ private TestIndexContext testIndex(MappingLookup mappingLookup, Client client) t
                 null,
                 () -> true,
                 null,
-                emptyMap(),
+                Map.of(),
                 MapperMetrics.NOOP
             );
 
@@ -630,7 +616,7 @@ private void runTestOnIndices(int numberIndices, CheckedConsumer<List<TestIndexC
             types.add(new MockFieldMapper(new KeywordFieldMapper.KeywordFieldType("dne-" + i)));
         }
 
-        MappingLookup mappingLookup = MappingLookup.fromMappers(Mapping.EMPTY, types, emptyList());
+        MappingLookup mappingLookup = MappingLookup.fromMappers(Mapping.EMPTY, types, List.of());
 
         final Client client = mock(Client.class);
         when(client.settings()).thenReturn(Settings.EMPTY);

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexReaderWrapperIntegrationTests.java

@@ -64,14 +64,11 @@
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.Executors;
 import java.util.stream.Collectors;
 
-import static java.util.Collections.emptyList;
-import static java.util.Collections.emptyMap;
-import static java.util.Collections.singleton;
-import static java.util.Collections.singletonMap;
 import static org.elasticsearch.xpack.core.security.SecurityField.DOCUMENT_LEVEL_SECURITY_FEATURE;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.equalTo;
@@ -115,7 +112,7 @@ public void testDLS() throws Exception {
             null,
             () -> true,
             null,
-            emptyMap(),
+            Map.of(),
             MapperMetrics.NOOP
         );
         SearchExecutionContext searchExecutionContext = spy(realSearchExecutionContext);
@@ -154,7 +151,7 @@ public void testDLS() throws Exception {
             if (doc % 11 == 0) {
                 iw.deleteDocuments(new Term("id", id));
             } else {
-                if (commitAfter % commitAfter == 0) {
+                if (doc % commitAfter == 0) {
                     iw.commit();
                 }
                 valuesHitCount[valueIndex]++;
@@ -172,7 +169,7 @@ public void testDLS() throws Exception {
             String termQuery = "{\"term\": {\"field\": \"" + values[i] + "\"} }";
             IndicesAccessControl.IndexAccessControl indexAccessControl = new IndicesAccessControl.IndexAccessControl(
                 FieldPermissions.DEFAULT,
-                DocumentPermissions.filteredBy(singleton(new BytesArray(termQuery)))
+                DocumentPermissions.filteredBy(Set.of(new BytesArray(termQuery)))
             );
             SecurityIndexReaderWrapper wrapper = new SecurityIndexReaderWrapper(
                 s -> searchExecutionContext,
@@ -184,7 +181,7 @@ public void testDLS() throws Exception {
 
                 @Override
                 protected IndicesAccessControl getIndicesAccessControl() {
-                    return new IndicesAccessControl(true, singletonMap("_index", indexAccessControl));
+                    return new IndicesAccessControl(true, Map.of("_index", indexAccessControl));
                 }
             };
 
@@ -237,9 +234,9 @@ public void testDLSWithLimitedPermissions() throws Exception {
             FieldPermissions.DEFAULT,
             DocumentPermissions.filteredBy(queries)
         );
-        queries = singleton(new BytesArray("{\"terms\" : { \"f1\" : [\"fv11\", \"fv21\", \"fv31\"] } }"));
+        queries = Set.of(new BytesArray("{\"terms\" : { \"f1\" : [\"fv11\", \"fv21\", \"fv31\"] } }"));
         if (restrictiveLimitedIndexPermissions) {
-            queries = singleton(new BytesArray("{\"terms\" : { \"f1\" : [\"fv11\", \"fv31\"] } }"));
+            queries = Set.of(new BytesArray("{\"terms\" : { \"f1\" : [\"fv11\", \"fv31\"] } }"));
         }
         IndicesAccessControl.IndexAccessControl limitedIndexAccessControl = new IndicesAccessControl.IndexAccessControl(
             FieldPermissions.DEFAULT,
@@ -271,7 +268,7 @@ public void testDLSWithLimitedPermissions() throws Exception {
             null,
             () -> true,
             null,
-            emptyMap(),
+            Map.of(),
             MapperMetrics.NOOP
         );
         SearchExecutionContext searchExecutionContext = spy(realSearchExecutionContext);
@@ -289,13 +286,13 @@ public void testDLSWithLimitedPermissions() throws Exception {
 
             @Override
             protected IndicesAccessControl getIndicesAccessControl() {
-                IndicesAccessControl indicesAccessControl = new IndicesAccessControl(true, singletonMap("_index", indexAccessControl));
+                IndicesAccessControl indicesAccessControl = new IndicesAccessControl(true, Map.of("_index", indexAccessControl));
                 if (noFilteredIndexPermissions) {
                     return indicesAccessControl;
                 }
                 IndicesAccessControl limitedByIndicesAccessControl = new IndicesAccessControl(
                     true,
-                    singletonMap("_index", limitedIndexAccessControl)
+                    Map.of("_index", limitedIndexAccessControl)
                 );
                 return indicesAccessControl.limitIndicesAccessControl(limitedByIndicesAccessControl);
             }
@@ -492,11 +489,7 @@ public void testDLSWithNestedDocs() throws Exception {
 
             @Override
             protected IndicesAccessControl getIndicesAccessControl() {
-                IndicesAccessControl indicesAccessControl = new IndicesAccessControl(
-                    true,
-                    singletonMap(indexSettings().getIndex().getName(), indexAccessControl)
-                );
-                return indicesAccessControl;
+                return new IndicesAccessControl(true, Map.of(indexSettings().getIndex().getName(), indexAccessControl));
             }
         };
 
@@ -522,6 +515,6 @@ protected IndicesAccessControl getIndicesAccessControl() {
 
     private static MappingLookup createMappingLookup(List<MappedFieldType> concreteFields) {
         List<FieldMapper> mappers = concreteFields.stream().map(MockFieldMapper::new).collect(Collectors.toList());
-        return MappingLookup.fromMappers(Mapping.EMPTY, mappers, emptyList());
+        return MappingLookup.fromMappers(Mapping.EMPTY, mappers, List.of());
     }
 }

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexReaderWrapperUnitTests.java

@@ -30,12 +30,12 @@
 import org.junit.After;
 import org.junit.Before;
 
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
-import static java.util.Collections.singletonMap;
 import static org.elasticsearch.xpack.core.security.SecurityField.DOCUMENT_LEVEL_SECURITY_FEATURE;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.sameInstance;
@@ -85,15 +85,15 @@ public void tearDown() throws Exception {
         esIn.close();
     }
 
-    public void testDefaultMetaFields() throws Exception {
+    public void testDefaultMetaFields() {
         securityIndexReaderWrapper = new SecurityIndexReaderWrapper(null, null, securityContext, licenseState, scriptService) {
             @Override
             protected IndicesAccessControl getIndicesAccessControl() {
                 IndicesAccessControl.IndexAccessControl indexAccessControl = new IndicesAccessControl.IndexAccessControl(
                     new FieldPermissions(fieldPermissionDef(new String[] {}, null)),
                     DocumentPermissions.allowAll()
                 );
-                return new IndicesAccessControl(true, singletonMap("_index", indexAccessControl));
+                return new IndicesAccessControl(true, Map.of("_index", indexAccessControl));
             }
         };
 
@@ -115,22 +115,22 @@ protected IndicesAccessControl getIndicesAccessControl() {
         assertThat(result.getFilter().run("some_random_regular_field"), is(false));
     }
 
-    public void testWrapReaderWhenFeatureDisabled() throws Exception {
+    public void testWrapReaderWhenFeatureDisabled() {
         when(licenseState.isAllowed(DOCUMENT_LEVEL_SECURITY_FEATURE)).thenReturn(false);
         securityIndexReaderWrapper = new SecurityIndexReaderWrapper(null, null, securityContext, licenseState, scriptService);
         DirectoryReader reader = securityIndexReaderWrapper.apply(esIn);
         assertThat(reader, sameInstance(esIn));
     }
 
-    public void testWildcards() throws Exception {
+    public void testWildcards() {
         Set<String> expected = new HashSet<>(META_FIELDS);
         expected.add("field1_a");
         expected.add("field1_b");
         expected.add("field1_c");
         assertResolved(new FieldPermissions(fieldPermissionDef(new String[] { "field1*" }, null)), expected, "field", "field2");
     }
 
-    public void testDotNotion() throws Exception {
+    public void testDotNotion() {
         Set<String> expected = new HashSet<>(META_FIELDS);
         expected.add("foo.bar");
         assertResolved(new FieldPermissions(fieldPermissionDef(new String[] { "foo.bar" }, null)), expected, "foo", "foo.baz", "bar.foo");
@@ -149,7 +149,7 @@ private void assertResolved(FieldPermissions permissions, Set<String> expected,
         }
     }
 
-    public void testFieldPermissionsWithFieldExceptions() throws Exception {
+    public void testFieldPermissionsWithFieldExceptions() {
         securityIndexReaderWrapper = new SecurityIndexReaderWrapper(null, null, securityContext, licenseState, null);
         String[] grantedFields = new String[] {};
         String[] deniedFields;
@@ -166,7 +166,7 @@ public void testFieldPermissionsWithFieldExceptions() throws Exception {
         deniedFields = META_FIELDS.toArray(new String[0]);
         assertResolved(
             new FieldPermissions(fieldPermissionDef(null, deniedFields)),
-            new HashSet<>(Arrays.asList("foo", "bar", "_some_plugin_meta_field"))
+            new HashSet<>(List.of("foo", "bar", "_some_plugin_meta_field"))
         );
 
         // check we can add all fields with *