Skip to content

Conversation

@fabriziofortino
Copy link
Contributor

No description provided.

@fabriziofortino fabriziofortino merged commit e44e9c6 into apache:trunk Jul 25, 2023
@fabriziofortino fabriziofortino deleted the OAK-10365 branch July 25, 2023 08:13
Comment on lines +282 to +283
return getOptionalValue(definition, PROP_INDEX_MAPPING_VERSION, "1.0.0");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the default value be a clearly invalid version, like 0.0.0? Because we will assume that 1.0.0 is a valid version, so it can create confusion in cases where the index is missing a version, we might assume wrongly that it has a version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mapping version is internal and cannot be changed in the index definition (it's stored in a hidden property). If the index definition does not have a version, we return the base version currently set as 1.0.0.

Comment on lines +30 to +39
@Test
public void indexStoresMappingVersion() throws Exception {
IndexDefinitionBuilder builder = createIndex("a").noAsync();
builder.indexRule("nt:base").property("a").propertyIndex();
Tree index = setIndex(UUID.randomUUID().toString(), builder);
root.commit();

assertEventually(() -> assertEquals(ElasticIndexHelper.MAPPING_VERSION,
getElasticIndexDefinition(index).getMappingVersion()));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is missing a test that sets the mapping version and checks that it returns the version that we set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in the comment above, the version is internal and cannot be configured in the index definition. In this test we just need to test the mapping version gets stored and is equal to the current mapping version (ElasticIndexHelper.MAPPING_VERSION).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants