Skip to content

Commit

Permalink
merge: #9854
Browse files Browse the repository at this point in the history
9854: Allow relaxed instantiation of exporter configuration r=npepinpe a=npepinpe

## Description

This PR fixes a bug where deserialization of exporter configuration was case sensitive, such that a property `NUMBEROFSHARDS` would not map to an instance field `numberOfShards`.

This isn't perfect, as now if we have two fields with the same name but different cases, only one of them will be set, using the latest value in the map (where the order is undefined). It should solve the most common cases however.

## Related issues

closes #7628 



Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>
  • Loading branch information
zeebe-bors-camunda[bot] and npepinpe committed Jul 22, 2022
2 parents 5685afa + 0fd1eeb commit b598714
Show file tree
Hide file tree
Showing 8 changed files with 83 additions and 54 deletions.
5 changes: 0 additions & 5 deletions broker/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,6 @@
<artifactId>jackson-datatype-jsr310</artifactId>
</dependency>

<dependency>
<groupId>com.google.code.gson</groupId>
<artifactId>gson</artifactId>
</dependency>

<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,25 +7,27 @@
*/
package io.camunda.zeebe.broker.exporter.context;

import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
import com.google.gson.JsonElement;
import io.camunda.zeebe.exporter.api.ExporterException;
import com.fasterxml.jackson.core.JsonParser.Feature;
import com.fasterxml.jackson.databind.MapperFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.json.JsonMapper;
import io.camunda.zeebe.exporter.api.context.Configuration;
import io.camunda.zeebe.util.ReflectUtil;
import java.util.Map;

public final class ExporterConfiguration implements Configuration {
private static final Gson CONFIG_INSTANTIATOR = new GsonBuilder().create();
public record ExporterConfiguration(String id, Map<String, Object> arguments)
implements Configuration {

private final String id;
private final Map<String, Object> arguments;

private JsonElement intermediateConfiguration;

public ExporterConfiguration(final String id, final Map<String, Object> arguments) {
this.id = id;
this.arguments = arguments;
}
// Accepts more lenient cases, such that the property "something" would match a field "someThing"
// Note however that if a field "something" and "someThing" are present, only one of them will be
// instantiated (the last declared one), using the last matching value.
private static final ObjectMapper MAPPER =
JsonMapper.builder()
.enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_ENUMS)
.enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_PROPERTIES)
.enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_VALUES)
.enable(Feature.IGNORE_UNDEFINED, Feature.ALLOW_SINGLE_QUOTES)
.build();

@Override
public String getId() {
Expand All @@ -40,25 +42,9 @@ public Map<String, Object> getArguments() {
@Override
public <T> T instantiate(final Class<T> configClass) {
if (arguments != null) {
return CONFIG_INSTANTIATOR.fromJson(getIntermediateConfiguration(), configClass);
return MAPPER.convertValue(arguments, configClass);
} else {
try {
return configClass.newInstance();
} catch (final Exception e) {
throw new ExporterException(
"Unable to instantiate config class "
+ configClass.getName()
+ " with default constructor",
e);
}
return ReflectUtil.newInstance(configClass);
}
}

private JsonElement getIntermediateConfiguration() {
if (intermediateConfiguration == null) {
intermediateConfiguration = CONFIG_INSTANTIATOR.toJsonTree(arguments);
}

return intermediateConfiguration;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,6 @@ public ExporterConfiguration getConfiguration() {
}

public String getId() {
return configuration.getId();
return configuration.id();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Copyright Camunda Services GmbH and/or licensed to Camunda Services GmbH under
* one or more contributor license agreements. See the NOTICE file distributed
* with this work for additional information regarding copyright ownership.
* Licensed under the Zeebe Community License 1.1. You may not use this file
* except in compliance with the Zeebe Community License 1.1.
*/
package io.camunda.zeebe.broker.exporter.context;

import static org.assertj.core.api.Assertions.assertThat;

import java.util.Map;
import org.junit.jupiter.api.parallel.Execution;
import org.junit.jupiter.api.parallel.ExecutionMode;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

@Execution(ExecutionMode.CONCURRENT)
final class ExporterConfigurationTest {
@ParameterizedTest
@ValueSource(strings = {"numberofshards", "numberOfShards", "NUMBEROFSHARDS"})
void shouldInstantiateConfigWithCaseInsensitiveProperties(final String property) {
// given
final var args = Map.<String, Object>of(property, 1);
final var expected = new Config(1);
final var config = new ExporterConfiguration("id", args);

// when
final var instance = config.instantiate(Config.class);

// then
assertThat(instance).isEqualTo(expected);
}

@ParameterizedTest
@ValueSource(strings = {"numberofshards", "numberOfShards", "NUMBEROFSHARDS"})
void shouldInstantiateNestedConfigWithCaseInsensitiveProperties(final String property) {
// given
final var args = Map.<String, Object>of("nested", Map.of(property, 1));
final var expected = new ContainerConfig(new Config(1));
final var config = new ExporterConfiguration("id", args);

// when
final var instance = config.instantiate(ContainerConfig.class);

// then
assertThat(instance).isEqualTo(expected);
}

private record Config(int numberOfShards) {}

private record ContainerConfig(Config nested) {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ void shouldLoadExternalExporter(final @TempDir File tempDir) throws Exception {

// then
assertThat(config.isExternal()).isTrue();
assertThat(descriptor.getConfiguration().getArguments()).isEqualTo(config.getArgs());
assertThat(descriptor.getConfiguration().getId()).isEqualTo("exported");
assertThat(descriptor.getConfiguration().arguments()).isEqualTo(config.getArgs());
assertThat(descriptor.getConfiguration().id()).isEqualTo("exported");
assertThat(descriptor.newInstance().getClass().getCanonicalName())
.isEqualTo(ExternalExporter.EXPORTER_CLASS_NAME);
}
Expand Down
13 changes: 7 additions & 6 deletions parent/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -606,12 +606,6 @@
<version>${version.guava}</version>
</dependency>

<dependency>
<groupId>com.google.code.gson</groupId>
<artifactId>gson</artifactId>
<version>${version.gson}</version>
</dependency>

<dependency>
<groupId>io.zeebe</groupId>
<artifactId>zeebe-test-container</artifactId>
Expand Down Expand Up @@ -924,6 +918,13 @@
<artifactId>hamcrest-core</artifactId>
<version>${version.hamcrest}</version>
</dependency>

<!-- grpc-core & protobuf-java-util require different versions of gson -->
<dependency>
<groupId>com.google.code.gson</groupId>
<artifactId>gson</artifactId>
<version>${version.gson}</version>
</dependency>
</dependencies>
</dependencyManagement>

Expand Down
6 changes: 0 additions & 6 deletions qa/integration-tests/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -251,12 +251,6 @@
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot</artifactId>
<scope>test</scope>
</dependency>

<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.boot.test.context.SpringBootTest.WebEnvironment;
import org.springframework.boot.web.server.LocalServerPort;
import org.springframework.boot.test.web.server.LocalServerPort;
import org.springframework.test.context.ActiveProfiles;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit.jupiter.SpringExtension;
Expand Down

0 comments on commit b598714

Please sign in to comment.