From 3d80bc4ae448f588267e4b9edb9ab954409441e8 Mon Sep 17 00:00:00 2001 From: Andreas Keefer Date: Sat, 27 Apr 2024 14:40:06 +0200 Subject: [PATCH] bugfix #293: trailing newlines will no longer be removed from migration scripts. --- .github/workflows/maven-matrix.yml | 4 +- README.md | 13 ++-- .../core/ElasticsearchEvolution.java | 3 +- .../input/MigrationScriptParserImpl.java | 64 ++++++++--------- .../input/MigrationScriptReaderImpl.java | 13 ++-- .../core/ElasticsearchEvolutionIT.java | 4 +- .../core/ElasticsearchEvolutionTest.java | 2 +- .../input/MigrationScriptParserImplTest.java | 70 +++++++++++++++++-- .../input/MigrationScriptReaderImplTest.java | 42 +++++++++++ .../test/EmbeddedElasticsearchExtension.java | 8 +-- ...iresNewlineAtTheEndOfTheMigrationFile.http | 6 ++ .../with_trailing_newline.http | 1 + tests/test-spring-boot-3.1/pom.xml | 2 +- tests/test-spring-boot-3.2/pom.xml | 2 +- 14 files changed, 168 insertions(+), 66 deletions(-) create mode 100644 elasticsearch-evolution-core/src/test/resources/es/ElasticsearchEvolutionTest/migrate_OK/V004.00__bulkAPIRequiresNewlineAtTheEndOfTheMigrationFile.http create mode 100644 elasticsearch-evolution-core/src/test/resources/scriptreader/issue293_trailing_newlines/with_trailing_newline.http diff --git a/.github/workflows/maven-matrix.yml b/.github/workflows/maven-matrix.yml index f47f1b98..34aaa70a 100644 --- a/.github/workflows/maven-matrix.yml +++ b/.github/workflows/maven-matrix.yml @@ -40,8 +40,8 @@ jobs: build-with-es: strategy: matrix: - elasticsearchVersion: [ "8.12.2", "8.11.4", "8.10.4", "8.9.2", "8.8.2", "8.7.1", "8.6.2", "8.5.3", "8.4.3", "8.3.3", "8.2.3", "8.1.3", "8.0.1", - "7.17.18", "7.5.2" ] + elasticsearchVersion: [ "8.13.0", "8.12.2", "8.11.4", "8.10.4", "8.9.2", "8.8.2", "8.7.1", "8.6.2", "8.5.3", "8.4.3", "8.3.3", "8.2.3", "8.1.3", "8.0.1", + "7.17.20", "7.5.2" ] fail-fast: false runs-on: ubuntu-22.04 steps: diff --git a/README.md b/README.md index f26fbb76..a83dc136 100644 --- a/README.md +++ b/README.md @@ -22,7 +22,7 @@ Successful executed migration scripts will not be executed again! - tested on Java 8, 11, 17, and 21 - runs on Spring-Boot 2.1, 2.2, 2.3, 2.4, 2.5, 2.6, 2.7, 3.0, 3.1 and 3.2 (and of course without Spring-Boot) -- runs on Elasticsearch version 7.5.x - 8.12.x +- runs on Elasticsearch version 7.5.x - 8.13.x - runs on Opensearch version 1.x and 2.x - highly configurable (e.g. location(s) of your migration files, migration files format pattern) - placeholder substitution in migration scripts @@ -33,16 +33,11 @@ Successful executed migration scripts will not be executed again! | Compatibility | Spring Boot | Elasticsearch | Opensearch | |----------------------------------|--------------------------------------------------|----------------------|------------| -| elasticsearch-evolution >= 0.4.2 | 2.1, 2.2, 2.3, 2.4, 2.5, 2.6, 2.7, 3.0, 3.1, 3.2 | 7.5.x - 8.12.x | 1.x - 2.x | +| elasticsearch-evolution >= 0.4.2 | 2.1, 2.2, 2.3, 2.4, 2.5, 2.6, 2.7, 3.0, 3.1, 3.2 | 7.5.x - 8.13.x | 1.x - 2.x | | elasticsearch-evolution >= 0.4.0 | 2.1, 2.2, 2.3, 2.4, 2.5, 2.6, 2.7 | 7.5.x - 8.6.x | 1.x - 2.x | | elasticsearch-evolution 0.3.x | 2.1, 2.2, 2.3, 2.4, 2.5, 2.6, 2.7 | 7.5.x - 7.17.x | | | elasticsearch-evolution 0.2.x | 1.5, 2.0, 2.1, 2.2, 2.3, 2.4 | 7.0.x - 7.4.x, 6.8.x | | -NOTE: When you run on Java 11 and using spring-boot 2.2 or 2.3 and you hit [this issue](https://github.com/ronmamo/reflections/issues/279), you have 2 options: - -- downgrade to `org.reflections:reflections:0.9.11` via defining this dependency explicitly -- use spring boot 2.3.6+ - ## 3 Quickstart ### 3.1 Quickstart with Spring-Boot starter @@ -293,7 +288,9 @@ ElasticsearchEvolution.configure() ### v0.5.2-SNAPSHOT -- ... +- bugfix ([#293](https://github.com/senacor/elasticsearch-evolution/issues/293)): trailing newlines will no longer be removed from migration scripts. +- added regression tests against OpenSearch 2.13 +- added regression tests against ElasticSearch 8.13 ### v0.5.1 diff --git a/elasticsearch-evolution-core/src/main/java/com/senacor/elasticsearch/evolution/core/ElasticsearchEvolution.java b/elasticsearch-evolution-core/src/main/java/com/senacor/elasticsearch/evolution/core/ElasticsearchEvolution.java index f45f33ed..2b182a98 100644 --- a/elasticsearch-evolution-core/src/main/java/com/senacor/elasticsearch/evolution/core/ElasticsearchEvolution.java +++ b/elasticsearch-evolution-core/src/main/java/com/senacor/elasticsearch/evolution/core/ElasticsearchEvolution.java @@ -140,7 +140,8 @@ protected MigrationScriptParser createMigrationScriptParser() { getConfig().getPlaceholders(), getConfig().getPlaceholderPrefix(), getConfig().getPlaceholderSuffix(), - getConfig().isPlaceholderReplacement() + getConfig().isPlaceholderReplacement(), + getConfig().getLineSeparator() ); } diff --git a/elasticsearch-evolution-core/src/main/java/com/senacor/elasticsearch/evolution/core/internal/migration/input/MigrationScriptParserImpl.java b/elasticsearch-evolution-core/src/main/java/com/senacor/elasticsearch/evolution/core/internal/migration/input/MigrationScriptParserImpl.java index 940e94b3..03a42d11 100644 --- a/elasticsearch-evolution-core/src/main/java/com/senacor/elasticsearch/evolution/core/internal/migration/input/MigrationScriptParserImpl.java +++ b/elasticsearch-evolution-core/src/main/java/com/senacor/elasticsearch/evolution/core/internal/migration/input/MigrationScriptParserImpl.java @@ -10,9 +10,6 @@ import com.senacor.elasticsearch.evolution.core.internal.model.migration.ParsedMigrationScript; import com.senacor.elasticsearch.evolution.core.internal.model.migration.RawMigrationScript; -import java.io.BufferedReader; -import java.io.IOException; -import java.io.StringReader; import java.util.Collection; import java.util.List; import java.util.Map; @@ -21,7 +18,6 @@ import static com.senacor.elasticsearch.evolution.core.internal.utils.AssertionUtils.requireCondition; import static com.senacor.elasticsearch.evolution.core.internal.utils.AssertionUtils.requireNotBlank; -import static java.lang.System.lineSeparator; import static java.util.Objects.requireNonNull; /** @@ -39,6 +35,7 @@ public class MigrationScriptParserImpl implements MigrationScriptParser { private final String placeholderPrefix; private final String placeholderSuffix; private final boolean placeholderReplacement; + private final String lineSeparator; /** * create Parser @@ -48,13 +45,15 @@ public MigrationScriptParserImpl(String esMigrationPrefix, Map placeholders, String placeholderPrefix, String placeholderSuffix, - boolean placeholderReplacement) { + boolean placeholderReplacement, + String lineSeparator) { this.esMigrationPrefix = esMigrationPrefix; this.esMigrationSuffixes = esMigrationSuffixes; this.placeholders = placeholders; this.placeholderPrefix = placeholderPrefix; this.placeholderSuffix = placeholderSuffix; this.placeholderReplacement = placeholderReplacement; + this.lineSeparator = lineSeparator; } @Override @@ -79,36 +78,31 @@ private MigrationScriptRequest parseContent(RawMigrationScript script) { : script.getContent(); MigrationScriptRequest res = new MigrationScriptRequest(); - try (BufferedReader reader = new BufferedReader(new StringReader(contentReplaced))) { - final AtomicReference state = new AtomicReference<>(ParseState.METHOD_PATH); - reader.lines() - // filter out comment lines - .filter(line -> !line.trim().startsWith("#") && !line.trim().startsWith("//")) - .forEachOrdered(line -> { - switch (state.get()) { - case METHOD_PATH: - parseMethodWithPath(res, line); - state.set(ParseState.HEADER); - break; - case HEADER: - if (line.trim().isEmpty()) { - state.set(ParseState.CONTENT); - } else { - parseHeader(res, line); - } - break; - case CONTENT: - if (!res.isBodyEmpty()) { - res.addToBody(lineSeparator()); - } - res.addToBody(line); - break; - default: - throw new UnsupportedOperationException("state '" + state + "' not supportet"); + final AtomicReference state = new AtomicReference<>(ParseState.METHOD_PATH); + for (String line : contentReplaced.split(lineSeparator, -1)) { + if (!line.trim().startsWith("#") && !line.trim().startsWith("//")) { + switch (state.get()) { + case METHOD_PATH: + parseMethodWithPath(res, line); + state.set(ParseState.HEADER); + break; + case HEADER: + if (line.trim().isEmpty()) { + state.set(ParseState.CONTENT); + } else { + parseHeader(res, line); } - }); - } catch (IOException e) { - throw new MigrationException("failed parsing content of " + script.getFileName(), e); + break; + case CONTENT: + if (!res.isBodyEmpty()) { + res.addToBody(lineSeparator); + } + res.addToBody(line); + break; + default: + throw new UnsupportedOperationException("state '" + state + "' not supportet"); + } + } } return res; @@ -125,7 +119,7 @@ private void parseHeader(MigrationScriptRequest res, String line) { } private void parseMethodWithPath(MigrationScriptRequest res, String line) { - String[] methodAndPath = line.trim().split("[ ]+", 2); + String[] methodAndPath = line.trim().split(" +", 2); if (methodAndPath.length != 2) { throw new MigrationException(String.format( "can't parse method and path: '%s'. Method and path must be separated by space and should look like this: 'PUT /my_index'", diff --git a/elasticsearch-evolution-core/src/main/java/com/senacor/elasticsearch/evolution/core/internal/migration/input/MigrationScriptReaderImpl.java b/elasticsearch-evolution-core/src/main/java/com/senacor/elasticsearch/evolution/core/internal/migration/input/MigrationScriptReaderImpl.java index 3e633001..d1ad0f0b 100644 --- a/elasticsearch-evolution-core/src/main/java/com/senacor/elasticsearch/evolution/core/internal/migration/input/MigrationScriptReaderImpl.java +++ b/elasticsearch-evolution-core/src/main/java/com/senacor/elasticsearch/evolution/core/internal/migration/input/MigrationScriptReaderImpl.java @@ -151,10 +151,15 @@ private Stream readScriptsFromClassPath(String location) { return res.stream(); } - private Stream read(BufferedReader reader, String filename) { - String content = reader.lines() - // use static line separator ('\n' per default) to get predictable and system independent checksum later - .collect(Collectors.joining(lineSeparator)); + Stream read(BufferedReader reader, String filename) throws IOException { + StringBuilder sb = new StringBuilder(); + int ch; + while ((ch = reader.read()) != -1) { + sb.append((char) ch); + } + // use static line separator ('\n' per default) to get predictable and system independent checksum later + String content = sb.toString().replaceAll("\\R", lineSeparator); + if (content.isEmpty()) { return Stream.empty(); } diff --git a/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/ElasticsearchEvolutionIT.java b/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/ElasticsearchEvolutionIT.java index 6d23f9c5..465ea68e 100644 --- a/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/ElasticsearchEvolutionIT.java +++ b/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/ElasticsearchEvolutionIT.java @@ -63,10 +63,10 @@ void migrate_OK(String versionInfo, EsUtils esUtils, RestHighLevelClient restHig assertSoftly(softly -> { softly.assertThat(underTest.migrate()) .as("# of successful executed scripts ") - .isEqualTo(7); + .isEqualTo(8); softly.assertThat(historyRepository.findAll()) .as("# of historyIndex entries and all are successful") - .hasSize(7) + .hasSize(8) .allMatch(MigrationScriptProtocol::isSuccess); }); esUtils.refreshIndices(); diff --git a/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/ElasticsearchEvolutionTest.java b/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/ElasticsearchEvolutionTest.java index b0634621..430528b3 100644 --- a/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/ElasticsearchEvolutionTest.java +++ b/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/ElasticsearchEvolutionTest.java @@ -49,7 +49,7 @@ void migrate_historyMaxQuerySizeToLow() throws IOException { assertThatThrownBy(underTest::migrate) .isInstanceOf(MigrationException.class) - .hasMessage("configured historyMaxQuerySize of '%s' is to low for the number of migration scripts of '%s'", historyMaxQuerySize, 7); + .hasMessage("configured historyMaxQuerySize of '%s' is to low for the number of migration scripts of '%s'", historyMaxQuerySize, 8); InOrder order = inOrder(restHighLevelClient, restClient); order.verify(restHighLevelClient, times(2)).getLowLevelClient(); diff --git a/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/internal/migration/input/MigrationScriptParserImplTest.java b/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/internal/migration/input/MigrationScriptParserImplTest.java index 3afc3365..1d72765a 100644 --- a/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/internal/migration/input/MigrationScriptParserImplTest.java +++ b/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/internal/migration/input/MigrationScriptParserImplTest.java @@ -36,7 +36,8 @@ void withOneReplacement_isReplaced() { Maps.newHashMap("index", "myIndex"), "${", "}", - true); + true, + "\n"); String replaced = parser.replaceParams(template); @@ -51,7 +52,8 @@ void withMultipleReplacementsOfSameKey_isReplaced() { Maps.newHashMap("index", "myIndex"), "${", "}", - true); + true, + "\n"); String replaced = parser.replaceParams(template); @@ -68,7 +70,8 @@ void withMultipleReplacementsOfDifferentKey_isReplaced() { placeholders, "${", "}", - true); + true, + "\n"); String replaced = parser.replaceParams(template); @@ -85,7 +88,8 @@ class parseFilename { null, null, null, - false); + false, + "\n"); @Test void isParsable() { @@ -147,7 +151,8 @@ class parseCollection { null, null, null, - false); + false, + "\n"); @Test void success() { @@ -186,7 +191,8 @@ class parseSingle { null, null, null, - false); + false, + "\n"); @Test void success_withMethodAndPathAndHeaderAndBody() { @@ -358,7 +364,8 @@ void success_replacePlaceholders() { placeholders, "${", "}", - true) + true, + "\n") .parse(new RawMigrationScript() .setFileName(fileName) .setContent(defaultContent)); @@ -391,6 +398,55 @@ void success_replacePlaceholders() { }); } + @Test + void success_NotRemoveTrailingNewlines() { + String defaultContent = "PUT /my-index" + lineSeparator() + + lineSeparator() + + "{" + lineSeparator() + "\"index\":\"my-index\"" + lineSeparator() + "}" + + lineSeparator(); + String fileName = "V1__create.http"; + HashMap placeholders = new HashMap<>(); + + ParsedMigrationScript res = new MigrationScriptParserImpl( + "V", + Collections.singletonList(".http"), + placeholders, + "${", + "}", + false, + "\n") + .parse(new RawMigrationScript() + .setFileName(fileName) + .setContent(defaultContent)); + + assertSoftly(softly -> { + softly.assertThat(res.getChecksum()) + .as("Checksum") + .isEqualTo(defaultContent.hashCode()); + softly.assertThat(res.getFileNameInfo().getVersion()) + .as("version") + .isEqualTo(MigrationVersion.fromVersion("1")); + softly.assertThat(res.getFileNameInfo().getScriptName()) + .as("scriptName") + .isEqualTo(fileName); + softly.assertThat(res.getFileNameInfo().getDescription()) + .as("description") + .isEqualTo("create"); + softly.assertThat(res.getMigrationScriptRequest().getHttpMethod()) + .as("methot") + .isEqualTo(HttpMethod.PUT); + softly.assertThat(res.getMigrationScriptRequest().getPath()) + .as("path") + .isEqualTo("/my-index"); + softly.assertThat(res.getMigrationScriptRequest().getHttpHeader()) + .as("header") + .isEmpty(); + softly.assertThat(res.getMigrationScriptRequest().getBody()) + .as("body") + .isEqualTo("{" + lineSeparator() + "\"index\":\"my-index\"" + lineSeparator() + "}" + lineSeparator()); + }); + } + @Test void failed_MethodAndPathInvalid() { String defaultContent = "/"; diff --git a/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/internal/migration/input/MigrationScriptReaderImplTest.java b/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/internal/migration/input/MigrationScriptReaderImplTest.java index 02e5d842..0217d734 100644 --- a/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/internal/migration/input/MigrationScriptReaderImplTest.java +++ b/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/internal/migration/input/MigrationScriptReaderImplTest.java @@ -4,8 +4,12 @@ import com.senacor.elasticsearch.evolution.core.internal.model.migration.RawMigrationScript; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import java.io.BufferedReader; import java.io.IOException; +import java.io.StringReader; import java.net.URISyntaxException; import java.net.URL; import java.nio.charset.StandardCharsets; @@ -138,6 +142,21 @@ void handle_locations_with_suffix() { new RawMigrationScript().setFileName("content_sub.http").setContent("sub content!")); } + @Test + void include_trailing_newlines() { + MigrationScriptReaderImpl reader = new MigrationScriptReaderImpl( + Arrays.asList("classpath:scriptreader/issue293_trailing_newlines"), + StandardCharsets.UTF_8, + "w", + singletonList(".http"), "\n"); + + List actual = reader.read(); + + assertThat(actual) + .containsExactlyInAnyOrder( + new RawMigrationScript().setFileName("with_trailing_newline.http").setContent("content!\n")); + } + @Test void withWrongProtocol() { MigrationScriptReaderImpl reader = new MigrationScriptReaderImpl( @@ -249,6 +268,29 @@ void validPathButNoFiles() throws URISyntaxException { } } + @ParameterizedTest + @ValueSource(strings = { + "foo\nbar", + "foo\r\nbar", + "foo\rbar" + }) + void read_should_normalize_new_lines_to_defined_line_separator(String input) throws IOException { + final String lineSeparator = ""; + MigrationScriptReaderImpl reader = new MigrationScriptReaderImpl( + singletonList("ignore"), + StandardCharsets.UTF_8, + "ignore", + singletonList(".ignore"), lineSeparator); + + final Stream res; + try (BufferedReader bufferedReader = new BufferedReader(new StringReader(input))) { + res = reader.read(bufferedReader, "filename"); + } + + assertThat(res) + .containsExactlyInAnyOrder(new RawMigrationScript().setFileName("filename").setContent("foo"+lineSeparator+"bar")); + } + private URL resolveURL(String path) { ClassLoader classLoader = getDefaultClassLoader(); if (classLoader != null) { diff --git a/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/test/EmbeddedElasticsearchExtension.java b/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/test/EmbeddedElasticsearchExtension.java index 65038775..82b19a8d 100644 --- a/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/test/EmbeddedElasticsearchExtension.java +++ b/elasticsearch-evolution-core/src/test/java/com/senacor/elasticsearch/evolution/core/test/EmbeddedElasticsearchExtension.java @@ -44,15 +44,15 @@ public class EmbeddedElasticsearchExtension implements TestInstancePostProcessor private static final Logger logger = LoggerFactory.getLogger(EmbeddedElasticsearchExtension.class); private static final Namespace NAMESPACE = Namespace.create(ExtensionContext.class); private static final SortedSet SUPPORTED_SEARCH_VERSIONS = Collections.unmodifiableSortedSet(new TreeSet<>(Arrays.asList( + ofOpensearch("2.13.0"), ofOpensearch("2.12.0"), ofOpensearch("2.11.1"), - ofOpensearch("2.10.0"), - ofOpensearch("1.3.15"), + ofOpensearch("1.3.16"), + ofElasticsearch("8.13.0"), ofElasticsearch("8.12.2"), ofElasticsearch("8.11.4"), - ofElasticsearch("8.10.4"), - ofElasticsearch("7.17.18") + ofElasticsearch("7.17.20") ))); @Override diff --git a/elasticsearch-evolution-core/src/test/resources/es/ElasticsearchEvolutionTest/migrate_OK/V004.00__bulkAPIRequiresNewlineAtTheEndOfTheMigrationFile.http b/elasticsearch-evolution-core/src/test/resources/es/ElasticsearchEvolutionTest/migrate_OK/V004.00__bulkAPIRequiresNewlineAtTheEndOfTheMigrationFile.http new file mode 100644 index 00000000..706b79fc --- /dev/null +++ b/elasticsearch-evolution-core/src/test/resources/es/ElasticsearchEvolutionTest/migrate_OK/V004.00__bulkAPIRequiresNewlineAtTheEndOfTheMigrationFile.http @@ -0,0 +1,6 @@ +POST _bulk +Content-Type: application/x-ndjson + +# https://github.com/senacor/elasticsearch-evolution/issues/293 +{ "index" : { "_index" : "test_2" } } +{ "b" : false } diff --git a/elasticsearch-evolution-core/src/test/resources/scriptreader/issue293_trailing_newlines/with_trailing_newline.http b/elasticsearch-evolution-core/src/test/resources/scriptreader/issue293_trailing_newlines/with_trailing_newline.http new file mode 100644 index 00000000..893b542e --- /dev/null +++ b/elasticsearch-evolution-core/src/test/resources/scriptreader/issue293_trailing_newlines/with_trailing_newline.http @@ -0,0 +1 @@ +content! diff --git a/tests/test-spring-boot-3.1/pom.xml b/tests/test-spring-boot-3.1/pom.xml index 8b2ac8a6..857198aa 100644 --- a/tests/test-spring-boot-3.1/pom.xml +++ b/tests/test-spring-boot-3.1/pom.xml @@ -5,7 +5,7 @@ org.springframework.boot spring-boot-starter-parent - 3.1.9 + 3.1.11 com.senacor.elasticsearch.evolution diff --git a/tests/test-spring-boot-3.2/pom.xml b/tests/test-spring-boot-3.2/pom.xml index 5dcb9451..7a036274 100644 --- a/tests/test-spring-boot-3.2/pom.xml +++ b/tests/test-spring-boot-3.2/pom.xml @@ -5,7 +5,7 @@ org.springframework.boot spring-boot-starter-parent - 3.2.3 + 3.2.5 com.senacor.elasticsearch.evolution