Skip to content

Commit

Permalink
Fix some edge cases configuration parsing
Browse files Browse the repository at this point in the history 
- Optional in getter but not in setter seems problematic
- Document config parsing better
- Properly handle empty values in REST Profile so no HTTP call is made
- Possibly related to #113
  • Loading branch information
@maxidorius
maxidorius committed Feb 11, 2019
1 parent 6d1c6ed commit bd4ccbc
Show file tree
Hide file tree
Showing 4 changed files with 127 additions and 69 deletions.
118 changes: 65 additions & 53 deletions src/main/java/io/kamax/mxisd/backend/rest/RestProfileProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
import io.kamax.mxisd.profile.JsonProfileResult;
import io.kamax.mxisd.profile.ProfileProvider;
import org.apache.commons.io.IOUtils;
import org.apache.commons.lang.StringUtils;
import org.apache.commons.lang3.StringUtils;
import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpPost;
import org.apache.http.client.utils.URIBuilder;
Expand All @@ -49,7 +49,7 @@

public class RestProfileProvider extends RestProvider implements ProfileProvider {

private transient final Logger log = LoggerFactory.getLogger(RestProfileProvider.class);
private static final Logger log = LoggerFactory.getLogger(RestProfileProvider.class);

public RestProfileProvider(RestBackendConfig cfg) {
super(cfg);
Expand All @@ -60,64 +60,71 @@ private <T> Optional<T> doRequest(
Function<RestBackendConfig.ProfileEndpoints, Optional<String>> endpoint,
Function<JsonProfileResult, Optional<T>> value
) {
return cfg.getEndpoints().getProfile()
// We get the endpoint
.flatMap(endpoint)
// We only continue if there is a value
.filter(StringUtils::isNotBlank)
// We use the endpoint
.flatMap(url -> {
try {
URIBuilder builder = new URIBuilder(url);
HttpPost req = new HttpPost(builder.build());
req.setEntity(new StringEntity(GsonUtil.get().toJson(new JsonProfileRequest(userId)), ContentType.APPLICATION_JSON));
try (CloseableHttpResponse res = client.execute(req)) {
int sc = res.getStatusLine().getStatusCode();
if (sc == 404) {
log.info("Got 404 - No result found");
return Optional.empty();
}

if (sc != 200) {
throw new InternalServerError("Unexpected backed status code: " + sc);
}

String body = IOUtils.toString(res.getEntity().getContent(), StandardCharsets.UTF_8);
if (StringUtils.isBlank(body)) {
log.warn("Backend response body is empty/blank, expected JSON object with profile key");
return Optional.empty();
}

Optional<JsonObject> pJson = GsonUtil.findObj(GsonUtil.parseObj(body), "profile");
if (!pJson.isPresent()) {
log.warn("Backend response body is invalid, expected JSON object with profile key");
return Optional.empty();
}

JsonProfileResult profile = gson.fromJson(pJson.get(), JsonProfileResult.class);
return value.apply(profile);
}
} catch (JsonSyntaxException | InvalidJsonException e) {
log.error("Unable to parse backend response as JSON", e);
throw new InternalServerError(e);
} catch (URISyntaxException e) {
log.error("Unable to build a valid request URL", e);
throw new InternalServerError(e);
} catch (IOException e) {
log.error("I/O Error during backend request", e);
throw new InternalServerError();
}
});
Optional<String> url = endpoint.apply(cfg.getEndpoints().getProfile());
if (!url.isPresent()) {
return Optional.empty();
}

try {
URIBuilder builder = new URIBuilder(url.get());
HttpPost req = new HttpPost(builder.build());
req.setEntity(new StringEntity(GsonUtil.get().toJson(new JsonProfileRequest(userId)), ContentType.APPLICATION_JSON));
try (CloseableHttpResponse res = client.execute(req)) {
int sc = res.getStatusLine().getStatusCode();
if (sc == 404) {
log.info("Got 404 - No result found");
return Optional.empty();
}

if (sc != 200) {
throw new InternalServerError("Unexpected backed status code: " + sc);
}

String body = IOUtils.toString(res.getEntity().getContent(), StandardCharsets.UTF_8);
if (StringUtils.isBlank(body)) {
log.warn("Backend response body is empty/blank, expected JSON object with profile key");
return Optional.empty();
}

Optional<JsonObject> pJson = GsonUtil.findObj(GsonUtil.parseObj(body), "profile");
if (!pJson.isPresent()) {
log.warn("Backend response body is invalid, expected JSON object with profile key");
return Optional.empty();
}

JsonProfileResult profile = gson.fromJson(pJson.get(), JsonProfileResult.class);
return value.apply(profile);
}
} catch (JsonSyntaxException | InvalidJsonException e) {
log.error("Unable to parse backend response as JSON", e);
throw new InternalServerError(e);
} catch (URISyntaxException e) {
log.error("Unable to build a valid request URL", e);
throw new InternalServerError(e);
} catch (IOException e) {
log.error("I/O Error during backend request", e);
throw new InternalServerError();
}
}

@Override
public Optional<String> getDisplayName(_MatrixID userId) {
return doRequest(userId, p -> Optional.ofNullable(p.getDisplayName()), profile -> Optional.ofNullable(profile.getDisplayName()));
return doRequest(userId, p -> {
if (StringUtils.isBlank(p.getDisplayName())) {
return Optional.empty();
}
return Optional.ofNullable(p.getDisplayName());
}, profile -> Optional.ofNullable(profile.getDisplayName()));
}

@Override
public List<_ThreePid> getThreepids(_MatrixID userId) {
return doRequest(userId, p -> Optional.ofNullable(p.getThreepids()), profile -> {
return doRequest(userId, p -> {
if (StringUtils.isBlank(p.getThreepids())) {
return Optional.empty();
}
return Optional.ofNullable(p.getThreepids());
}, profile -> {
List<_ThreePid> t = new ArrayList<>();
if (Objects.nonNull(profile.getThreepids())) {
t.addAll(profile.getThreepids());
Expand All @@ -128,7 +135,12 @@ public List<_ThreePid> getThreepids(_MatrixID userId) {

@Override
public List<String> getRoles(_MatrixID userId) {
return doRequest(userId, p -> Optional.ofNullable(p.getRoles()), profile -> {
return doRequest(userId, p -> {
if (StringUtils.isBlank(p.getRoles())) {
return Optional.empty();
}
return Optional.ofNullable(p.getRoles());
}, profile -> {
List<String> t = new ArrayList<>();
if (Objects.nonNull(profile.getRoles())) {
t.addAll(profile.getRoles());
Expand Down
9 changes: 7 additions & 2 deletions src/main/java/io/kamax/mxisd/config/YamlConfigLoader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,14 @@ public static MxisdConfig loadFromFile(String path) throws IOException {
rep.getPropertyUtils().setSkipMissingProperties(true);
Yaml yaml = new Yaml(new Constructor(MxisdConfig.class), rep);
try (FileInputStream is = new FileInputStream(path)) {
Object o = yaml.load(is);
MxisdConfig raw = yaml.load(is);
log.debug("Read config in memory from {}", path);
MxisdConfig cfg = GsonUtil.get().fromJson(GsonUtil.get().toJson(o), MxisdConfig.class);

// SnakeYaml set objects to null when there is no value set in the config, even a full sub-tree.
// This is problematic for default config values and objects, to avoid NPEs.
// Therefore, we'll use Gson to re-parse the data in a way that avoids us checking the whole config for nulls.
MxisdConfig cfg = GsonUtil.get().fromJson(GsonUtil.get().toJson(raw), MxisdConfig.class);

log.info("Loaded config from {}", path);
return cfg;
}
Expand Down
12 changes: 8 additions & 4 deletions src/main/java/io/kamax/mxisd/config/rest/RestBackendConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
import java.net.MalformedURLException;
import java.net.URL;
import java.util.Objects;
import java.util.Optional;

public class RestBackendConfig {

Expand Down Expand Up @@ -118,8 +117,8 @@ public void setIdentity(IdentityEndpoints identity) {
this.identity = identity;
}

public Optional<ProfileEndpoints> getProfile() {
return Optional.ofNullable(profile);
public ProfileEndpoints getProfile() {
return profile;
}

public void setProfile(ProfileEndpoints profile) {
Expand All @@ -128,7 +127,7 @@ public void setProfile(ProfileEndpoints profile) {

}

private transient final Logger log = LoggerFactory.getLogger(RestBackendConfig.class);
private static final Logger log = LoggerFactory.getLogger(RestBackendConfig.class);

private boolean enabled;
private String host;
Expand Down Expand Up @@ -197,6 +196,11 @@ public void build() {
log.info("Directory endpoint: {}", endpoints.getDirectory());
log.info("Identity Single endpoint: {}", endpoints.identity.getSingle());
log.info("Identity Bulk endpoint: {}", endpoints.identity.getBulk());

log.info("Profile endpoints:");
log.info(" - Display name: {}", getEndpoints().getProfile().getDisplayName());
log.info(" - 3PIDs: {}", getEndpoints().getProfile().getThreepids());
log.info(" - Roles: {}", getEndpoints().getProfile().getRoles());
}
}

Expand Down
57 changes: 47 additions & 10 deletions src/test/java/io/kamax/mxisd/test/backend/rest/RestProfileProviderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.github.tomakehurst.wiremock.junit.WireMockRule;
import io.kamax.matrix.MatrixID;
import io.kamax.matrix._MatrixID;
import io.kamax.matrix._ThreePid;
import io.kamax.matrix.json.GsonUtil;
import io.kamax.mxisd.backend.rest.RestProfileProvider;
import io.kamax.mxisd.config.rest.RestBackendConfig;
Expand All @@ -34,6 +35,7 @@
import org.junit.Rule;
import org.junit.Test;

import java.util.List;
import java.util.Optional;

import static com.github.tomakehurst.wiremock.client.WireMock.*;
Expand All @@ -42,27 +44,40 @@

public class RestProfileProviderTest {

private static final int MockHttpPort = 65000;
private static final String MockHttpHost = "localhost";

@Rule
public WireMockRule wireMockRule = new WireMockRule(65000);
public WireMockRule wireMockRule = new WireMockRule(MockHttpPort);

private final String displayNameEndpoint = "/displayName";

private final _MatrixID userId = MatrixID.from("john", "matrix.localhost").valid();
private final _MatrixID userId = MatrixID.from("john", "matrix." + MockHttpHost).valid();

private RestProfileProvider p;

@Before
public void before() {
ProfileEndpoints endpoints = new ProfileEndpoints();
endpoints.setDisplayName(displayNameEndpoint);

private RestBackendConfig getCfg(RestBackendConfig.Endpoints endpoints) {
RestBackendConfig cfg = new RestBackendConfig();
cfg.setEnabled(true);
cfg.setHost("http://localhost:65000");
cfg.getEndpoints().setProfile(endpoints);
cfg.setHost("http://" + MockHttpHost + ":" + MockHttpPort);
cfg.setEndpoints(endpoints);
cfg.build();

p = new RestProfileProvider(cfg);
return cfg;
}

private RestProfileProvider get(RestBackendConfig cfg) {
return new RestProfileProvider(cfg);
}

@Before
public void before() {
ProfileEndpoints pEndpoints = new ProfileEndpoints();
pEndpoints.setDisplayName(displayNameEndpoint);
RestBackendConfig.Endpoints endpoints = new RestBackendConfig.Endpoints();
endpoints.setProfile(pEndpoints);

p = get(getCfg(endpoints));
}

@Test
Expand Down Expand Up @@ -144,4 +159,26 @@ public void forNameInvalidBody() {
}
}

@Test
public void forEmptyEndpoints() {
ProfileEndpoints pEndpoints = new ProfileEndpoints();
pEndpoints.setDisplayName("");
pEndpoints.setThreepids("");
pEndpoints.setRoles("");

RestBackendConfig.Endpoints endpoints = new RestBackendConfig.Endpoints();
endpoints.setProfile(pEndpoints);

RestProfileProvider p = get(getCfg(endpoints));

Optional<String> dn = p.getDisplayName(userId);
assertFalse(dn.isPresent());

List<String> roles = p.getRoles(userId);
assertTrue(roles.isEmpty());

List<_ThreePid> tpids = p.getThreepids(userId);
assertTrue(tpids.isEmpty());
}

}

0 comments on commit bd4ccbc

Please sign in to comment.