codeconsole commented on code in PR #16494:
URL: https://github.com/apache/grails-core/pull/16494#discussion_r4175194705
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -902,6 +979,174 @@ private void buildDeclaredIndexes(IndexBuildSummary
summary) {
}
}
+ /**
+ * The entities whose declared indexes this datastore builds.
+ */
+ private List<PersistentEntity> indexedEntities() {
+ List<PersistentEntity> entities = new ArrayList<>();
+ for (PersistentEntity entity :
this.mappingContext.getPersistentEntities()) {
+ // Only create Mongo templates for entities that are mapped with
Mongo
+ if (!entity.isExternal() &&
+ !(entity.isMultiTenant() && multiTenancyMode ==
MultiTenancySettings.MultiTenancyMode.SCHEMA)) {
+ entities.add(entity);
+ }
+ }
+ return entities;
+ }
+
+ /**
+ * The collections whose declared indexes this datastore builds, each with
the declarations of every class
+ * mapped to it: every class in an inheritance hierarchy maps to its
root's collection, and several classes
+ * can name the same one.
+ */
+ private Map<MongoNamespace, CollectionDeclarations>
declarationsByCollection() {
+ Map<MongoNamespace, CollectionDeclarations> collections = new
LinkedHashMap<>();
+ for (PersistentEntity entity : indexedEntities()) {
+ com.mongodb.client.MongoCollection<Document> collection =
getCollection(entity);
+ CollectionDeclarations declarations =
collections.computeIfAbsent(collection.getNamespace(),
+ namespace -> new CollectionDeclarations(collection, new
ArrayList<>()));
+ for (IndexDeclaration declaration : declaredIndexes(entity)) {
+ declarations.declarations().add(new EntityDeclaration(entity,
declaration));
+ }
+ }
+ return collections;
+ }
+
+ private record
CollectionDeclarations(com.mongodb.client.MongoCollection<Document> collection,
+ List<EntityDeclaration>
declarations) {
+ }
+
+ private record EntityDeclaration(PersistentEntity entity, IndexDeclaration
declaration) {
+ }
+
+ /**
+ * Lists the indexes the domain classes declare that their collections do
not have, on the collections
+ * whose declared indexes {@link #buildIndex()} builds for this
datastore's connection.
+ *
+ * <p>A declared index is present when its collection has an index on the
same key pattern, the same fields
+ * in the same order, whatever its name and options: an option that
differs is the build's to reconcile.
+ * Keys that several classes mapped to one collection declare are reported
once. Nothing is changed:
+ * {@link #buildIndex()} or {@link #buildIndexAsync()} creates them. Each
named connection has its own
+ * domain classes, so call this on {@link
#getDatastoreForConnection(String)} for those.
+ *
+ * @return the missing indexes, collection by collection, in the order the
domain classes declare them
+ */
+ public List<MissingIndex> findMissingIndexes() {
+ List<MissingIndex> missing = new ArrayList<>();
+ for (Map.Entry<MongoNamespace, CollectionDeclarations> entry :
declarationsByCollection().entrySet()) {
+ MongoNamespace namespace = entry.getKey();
+ List<Document> existing =
entry.getValue().collection().listIndexes().into(new ArrayList<>());
+ List<Document> reported = new ArrayList<>();
+ for (EntityDeclaration declared : entry.getValue().declarations())
{
+ Document keys = declared.declaration().keys();
+ if (findIndexByKeyPattern(existing, keys) != null ||
matchesAny(keys, reported)) {
+ continue;
+ }
+ reported.add(keys);
+ Map<String, Object> options = declared.declaration().options();
+ missing.add(new MissingIndex(namespace.getDatabaseName(),
namespace.getCollectionName(),
+ declared.entity().getName(), keys, options != null ?
new Document(options) : new Document()));
+ }
+ }
+ return missing;
+ }
+
+ /**
+ * Lists the indexes that no domain class declares, on the collections
whose declared indexes
+ * {@link #buildIndex()} builds for this datastore's connection.
+ *
+ * <p>An index is declared when a domain class mapped to the same
collection declares its key pattern, the
+ * same fields in the same order, with {@code compoundIndex}, {@code
index} or a property's
+ * {@code index: true}. Its name and options do not matter. The {@code
_id} index is never reported, and
+ * neither is any index on a collection that no domain class maps. An
index that an
+ * {@link #initializeIndices(PersistentEntity)} override creates by itself
is not a declaration, so it is
+ * reported.
+ *
+ * <p>Nothing is changed: see {@link #dropUndeclaredIndexes()}. Each named
connection has its own domain
+ * classes, so call this on {@link #getDatastoreForConnection(String)} for
those.
+ *
+ * @return the undeclared indexes, collection by collection, in the order
the server lists them
+ */
+ public List<UndeclaredIndex> findUndeclaredIndexes() {
+ List<UndeclaredIndex> undeclared = new ArrayList<>();
+ for (Map.Entry<MongoNamespace, CollectionDeclarations> entry :
declarationsByCollection().entrySet()) {
+ MongoNamespace namespace = entry.getKey();
+ List<Document> declared = new ArrayList<>();
+ for (EntityDeclaration declaration :
entry.getValue().declarations()) {
+ declared.add(declaration.declaration().keys());
+ }
+ for (Document index : entry.getValue().collection().listIndexes())
{
+ if (ID_INDEX_NAME.equals(index.getString("name"))) {
+ continue;
+ }
+ if (index.get("key") instanceof Document key &&
!matchesAny(key, declared)) {
+ undeclared.add(new
UndeclaredIndex(namespace.getDatabaseName(), namespace.getCollectionName(),
+ index.getString("name"), key, index));
+ }
+ }
+ }
+ return undeclared;
+ }
+
+ /**
+ * Drops every index that {@link #findUndeclaredIndexes()} reports.
+ *
+ * <p>Run this deliberately, once every instance of the application runs
the release whose domain classes
+ * declare the indexes to keep. An instance still on an earlier release
does not declare what a later
+ * release added, nor an index created by hand ahead of a deployment, so
to it those indexes are
+ * undeclared and this drops them.
+ *
+ * @return the indexes dropped
+ */
+ public List<UndeclaredIndex> dropUndeclaredIndexes() {
+ return dropUndeclaredIndexes(findUndeclaredIndexes());
+ }
+
+ /**
+ * Drops the given indexes: those {@link #findUndeclaredIndexes()}
reported, once they have been reviewed.
+ * An index that no longer exists, or whose collection no longer does, is
skipped.
+ *
+ * @param indexes the indexes to drop
+ * @return the indexes dropped
+ */
+ public List<UndeclaredIndex> dropUndeclaredIndexes(List<UndeclaredIndex>
indexes) {
+ List<UndeclaredIndex> dropped = new ArrayList<>();
+ // The driver answers a dropIndex on a collection that no longer
exists as a success, so what is still
+ // there is listed first, once per collection, rather than inferred
from the drop.
+ Map<MongoNamespace, Set<String>> present = new HashMap<>();
+ for (UndeclaredIndex index : indexes) {
+ com.mongodb.client.MongoCollection<Document> collection =
+
getMongoClient().getDatabase(index.database()).getCollection(index.collection());
+ Set<String> names =
present.computeIfAbsent(collection.getNamespace(),
+ namespace -> collection.listIndexes().map(listed ->
listed.getString("name")).into(new HashSet<>()));
+ if (!names.contains(index.name()) || !dropIndex(collection,
index.name())) {
Review Comment:
Fixed in cb9e0c4edc. `dropUndeclaredIndexes(List)` now lists each collection
with full definitions and drops an entry only while the index of that name is
on the reviewed keys (for a text index, the same text fields from `weights`),
its collection is mapped on this datastore's connection, and no class mapped to
it declares those keys. An entry that fails a check is skipped and logged at
`WARN`; `findUndeclaredIndexes()` and the drop share the declared check.
Regressions in `UndeclaredIndexesSpec`: a reviewed `maintenance_idx` on
`{code: 1}` replaced under that name by the declared unique `{name: 1}` is not
dropped; a list reviewed on a release that did not declare an index and dropped
on one that does is not dropped; and an entry on a collection the connection
does not map is not dropped.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -902,6 +979,174 @@ private void buildDeclaredIndexes(IndexBuildSummary
summary) {
}
}
+ /**
+ * The entities whose declared indexes this datastore builds.
+ */
+ private List<PersistentEntity> indexedEntities() {
+ List<PersistentEntity> entities = new ArrayList<>();
+ for (PersistentEntity entity :
this.mappingContext.getPersistentEntities()) {
+ // Only create Mongo templates for entities that are mapped with
Mongo
+ if (!entity.isExternal() &&
Review Comment:
Fixed in 5a261296a1. `indexedEntities()` now keeps only the classes
`ConnectionSourcesSupport.usesConnectionSource(entity, connectionName())`
accepts. It feeds the build as well as both reports, so a named connection's
build also stops creating the indexes (and the collection) of a class mapped
only to `default` in its database. That changes released behavior, so it has
upgrade notes in the Mongo guide and `upgrading80x.adoc`.
Tests in `UndeclaredIndexesSpec` and `MissingIndexesSpec` with a
default-only, a reporting-only and an `ALL` class: each connection reports only
its own classes' collections, the build puts each declared index only where its
class is mapped, and `dropUndeclaredIndexes()` on reporting leaves a same-named
collection of the default-only class alone.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -1590,6 +1862,13 @@ else if (reconciliation == Reconciliation.RECREATED) {
LOG.error("Failed to create index for entity [{}] {}: {}",
entity.getName(), descriptor, e.getMessage(), e);
}
+ } catch (MongoServerException e) {
Review Comment:
Fixed in dbbb0aab57. Both catches now count only a duplicate key
(`DuplicateKeyException`, or a code in `ErrorCategory.DUPLICATE_KEY`) and
rethrow any other non-command `MongoServerException`, so the build stops and
the caller or the future gets it. On the recreate path, if that happens after
the old index was dropped, a `WARN` says the outcome of the new index is not
known instead of the "no index on these keys" error. The existing
`MongoCommandException` handling is unchanged.
Tests in `BuildIndexAsyncResultSpec`, using `FailingMongoClient` so
`createIndex` is applied and then throws `MongoWriteConcernException`: the
future completes exceptionally and `buildIndex()` throws; and with
`recreateOnConflict`, after the old index was dropped, the build fails, the
unique index exists, and no "could not be built again" line is logged.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -703,26 +712,12 @@ public void buildIndex() {
connection);
return;
}
- ExecutorService executor = this.indexBuildExecutor;
- if (executor == null) {
- runIndexBuild(null);
+ if (!buildIndexesAsync) {
Review Comment:
Fixed in a9ea86909e. Every build holds one `ReentrantLock` per datastore, so
per connection, while it applies declarations, on the executor or the caller's
thread, and its elapsed time starts once it holds it. The wait is
interruptible, so `stop()` and `close()` still end a background build that is
waiting; a caller interrupted while waiting gets a `MongoInterruptedException`,
and a synchronous build still throws to its caller.
Latch tests in `BuildIndexAsyncResultSpec` for both directions:
`buildIndex()` does not enter index creation while a `buildIndexAsync()` build
is held inside it, and a `buildIndexAsync()` build does not while a
`buildIndex()` is; at most one is ever inside at once.
##########
grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java:
##########
@@ -1699,31 +1988,42 @@ private static Document
findIndexByName(Iterable<Document> indexes, String name)
/**
* Find an existing index whose key pattern matches the given keys, or
{@code null} if none.
- * Directions/types are compared numerically (1 vs 1.0) so driver-returned
values match.
- *
- * <p>Text indexes are special-cased: a declared text index has key {@code
{field: 'text'}}, but
- * MongoDB reports an existing one with a synthetic {@code {_fts: 'text',
_ftsx: 1}} key, so the
- * two never match by pattern. Since MongoDB allows at most one text index
per collection, an
- * existing text index is unambiguously the one a newly-declared text
index conflicts with —
- * match it regardless of its key shape or name so {@code
recreateOnConflict} can absorb it.</p>
*/
private static Document findIndexByKeyPattern(Iterable<Document> indexes,
Document keys) {
- boolean desiredIsText = isTextIndex(keys);
for (Document idx : indexes) {
- Object key = idx.get("key");
- if (!(key instanceof Document)) {
- continue;
- }
- if (desiredIsText && isTextIndex((Document) key)) {
- return idx;
- }
- if (sameKeyPattern((Document) key, keys)) {
+ if (idx.get("key") instanceof Document key &&
matchesDeclaration(key, keys)) {
return idx;
}
}
return null;
}
+ private static boolean matchesAny(Document key, List<Document>
declaredKeys) {
+ for (Document keys : declaredKeys) {
+ if (matchesDeclaration(key, keys)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * Whether an existing index's key pattern is the one a declaration
describes. Directions/types are
+ * compared numerically (1 vs 1.0) so driver-returned values match.
+ *
+ * <p>Text indexes are special-cased: a declared text index has key {@code
{field: 'text'}}, but
+ * MongoDB reports an existing one with a synthetic {@code {_fts: 'text',
_ftsx: 1}} key, so the
+ * two never match by pattern. Since MongoDB allows at most one text index
per collection, an
+ * existing text index is unambiguously the one a declared text index
describes or conflicts with —
+ * match it regardless of its key shape or name so {@code
recreateOnConflict} can absorb it.</p>
+ */
+ private static boolean matchesDeclaration(Document existingKey, Document
declaredKeys) {
+ if (isTextIndex(declaredKeys) && isTextIndex(existingKey)) {
Review Comment:
Fixed in a4d30e4b1c (plus 97dfcc7103 for checkstyle). Reconciliation keeps
the broad match. The reports compare a text index's fields, read from the
listing's `weights`, as a set, and the keys before and after them in order;
weights and options are ignored.
Tests in `MissingIndexesSpec` and `UndeclaredIndexesSpec`: a text index on
another field and one without the declared compound prefix are
missing/undeclared, and one on the declared fields and keys under another name
and other weights still matches.
##########
grails-data-mongodb/docs/src/docs/asciidoc/querying/queryIndexes.adoc:
##########
@@ -268,6 +281,8 @@ The split between created and already present is what makes
that time interpreta
Index build for database [myDb] finished in 9315ms: 0 created, 1 recreated, 6
already present, from 3 domain class(es)
----
+A declaration the server refuses does not end the build: an option conflict
GORM is not authorised to resolve, an invalid specification, or a `unique`
index over documents that already share a value is logged at `ERROR` as it
fails and counted, and the build goes on with the remaining declarations. Only
an error that stops the build itself, such as a lost connection, ends it early,
and with the default synchronous build that fails startup.
Review Comment:
Done in 8e44dec475. The background-build bullet now separates an error that
stops the build, which fails startup only with the synchronous build, from a
refused declaration, which fails it with neither setting. "Building Indexes on
Demand" now has the recipe: wait on `buildIndexAsync()` in `BootStrap` and
throw when `failures()` is not zero, before the application reports itself
ready.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
