jamesfredley commented on code in PR #16494:
URL: https://github.com/apache/grails-core/pull/16494#discussion_r4174992494
##########
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:
`MongoServerException` is not "the server refused this one index." Driver
5.12 also puts `MongoWriteConcernException` under it. A replication
acknowledgement failure is now counted as a failed declaration and the future
completes normally, instead of stopping the build. The same catch at line 1946
is worse: `recreateOnConflict` may already have dropped the old index, and the
log then says the collection has no index on those keys even when the create
applied and only the acknowledgement failed.
Handle duplicate-key failures specifically. Propagate operational and
indeterminate failures through the build and the future, including the recreate
path. Add a test where write concern fails after the old index was 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:
This does not filter by connection. Child datastores share the mapping
context, and this check only skips external entities and schema multi-tenancy.
A class mapped only to `default` is therefore also treated as mapped on
`reporting`. `findUndeclaredIndexes()` and `dropUndeclaredIndexes()` on that
connection can then drop indexes on a same-named collection this application
does not map there. The new named-connection tests use `ConnectionSource.ALL`,
which hides this.
Filter with `ConnectionSourcesSupport.usesConnectionSource(entity,
connectionName())` before collecting declarations. Test a default-only entity,
a reporting-only entity, and an `ALL` entity, including that an otherwise
unmapped collection is left alone.
##########
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:
Non-blocking. This paragraph is right: a refused declaration is counted and
the build continues. Line 259 still says that with the default synchronous
build the exception propagates and the application does not start. That is no
longer true for a duplicate-key refusal or another counted declaration failure.
Distinguish a fatal build failure from a counted declaration failure. The
fail-startup recipe should wait on `buildIndexAsync()` and reject a nonzero
`failures()` before accepting traffic.
##########
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:
With `buildIndexesAsync` off, this runs the build on the caller while
`buildIndexAsync()` (line 765) submits it to the executor. Both can run at once
on the same connection, including two `recreateOnConflict` drop-and-recreate
sequences. That contradicts the new contract at line 742 that builds on a
connection run one at a time.
Serialize synchronous and asynchronous builds with one per-connection
mechanism, and keep synchronous exception propagation. Add a latch test that a
`buildIndex()` call cannot enter index creation while a `buildIndexAsync()`
build is running.
##########
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:
A reviewed list is matched by name only. This re-lists names, not key
patterns, and then drops `index.name()` if that name is still present. If an
undeclared index named `maintenance_idx` is removed and replaced by a declared
index on different keys under the same name, passing the earlier list drops the
replacement, including a unique index `buildIndex()` just created with
`recreateOnConflict`.
Re-list the full definitions, reject an entry whose key pattern changed, and
verify the current index is still undeclared on a mapped collection before
dropping it. Add a regression that replaces a reviewed index with a declared
index under the same name, then calls this overload.
##########
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:
Any text index matches any text declaration. An existing text index on
`title` satisfies a declaration on `description`, and compound prefixes and
suffixes are ignored. `findMissingIndexes()` can then report nothing missing,
and `findUndeclaredIndexes()` keeps the obsolete index. Treating the one text
index per collection as a conflict candidate is right for `recreateOnConflict`.
It is the wrong comparison for these reports.
Keep the broad match for reconciliation. For the maintenance reports,
compare the text fields from the server definition's `weights` and the non-text
keys. Ignore weight values and options, as intended. Test a changed text field
and a compound prefix.
--
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]
