Skip to content
This repository has been archived by the owner on Jun 21, 2022. It is now read-only.

Handle errors in ApiRepoUpdateSlice #45

Merged
merged 3 commits into from
Aug 18, 2021
Merged

Handle errors in ApiRepoUpdateSlice #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6987f83
Handle errors properly in `ApiRepoUpdateSlice`
Aug 18, 2021
0373b26
checkstyle fixes
Aug 18, 2021
a3e07af
code review
Aug 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,12 +103,12 @@ SOFTWARE.
<dependency>
<groupId>com.artipie</groupId>
<artifactId>http</artifactId>
<version>0.18.4</version>
<version>v0.19</version>
</dependency>
<dependency>
<groupId>com.artipie</groupId>
<artifactId>asto</artifactId>
<version>1.1.0</version>
<version>v1.3.4</version>
</dependency>
<dependency>
<groupId>com.artipie</groupId>
Expand Down
40 changes: 30 additions & 10 deletions src/main/java/com/artipie/management/api/ApiRepoUpdateSlice.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import com.amihaiemil.eoyaml.YamlMapping;
import com.amihaiemil.eoyaml.YamlMappingBuilder;
import com.amihaiemil.eoyaml.YamlNode;
import com.artipie.ArtipieException;
import com.artipie.asto.Content;
import com.artipie.asto.Key;
import com.artipie.asto.ext.PublisherAs;
Expand All @@ -18,6 +19,7 @@
import com.artipie.http.async.AsyncResponse;
import com.artipie.http.rq.RequestLineFrom;
import com.artipie.http.rs.RsStatus;
import com.artipie.http.rs.RsWithBody;
import com.artipie.http.rs.RsWithHeaders;
import com.artipie.http.rs.RsWithStatus;
import com.artipie.management.ConfigFiles;
Expand All @@ -36,6 +38,7 @@
* @checkstyle ExecutableStatementCountCheck (500 lines)
* @checkstyle ClassDataAbstractionCouplingCheck (500 lines)
* @checkstyle CyclomaticComplexityCheck (500 lines)
* @checkstyle NPathComplexityCheck (500 lines)
*/
public final class ApiRepoUpdateSlice implements Slice {

Expand All @@ -58,7 +61,7 @@ public ApiRepoUpdateSlice(final ConfigFiles configfile) {
}

@Override
@SuppressWarnings("PMD.AvoidDuplicateLiterals")
@SuppressWarnings({"PMD.AvoidDuplicateLiterals", "PMD.NPathComplexity"})
public Response response(final String line,
final Iterable<Map.Entry<String, String>> headers, final Publisher<ByteBuffer> body) {
final Matcher matcher = PTN.matcher(new RequestLineFrom(line).uri().getPath());
Expand All @@ -76,14 +79,17 @@ public Response response(final String line,
.readYamlMapping()
).value();
final YamlMapping repo = config.yamlMapping("repo");
if (repo == null) {
throw new ArtipieException("Repo section is required");
}
final YamlNode type = repo.value("type");
if (type == null || !Scalar.class.isAssignableFrom(type.getClass())) {
throw new IllegalStateException("Repository type required");
throw new ArtipieException("Repository type required");
}
final YamlMapping ystor = repo.yamlMapping("storage");
final String sstor = repo.string("storage");
if (ystor == null && sstor == null) {
throw new IllegalStateException("Repository storage is required");
throw new ArtipieException("Repository storage is required");
}
YamlMappingBuilder yrepo = Yaml.createYamlMappingBuilder().add("type", type);
if (ystor == null) {
Expand All @@ -104,13 +110,27 @@ public Response response(final String line,
Yaml.createYamlMappingBuilder().add("repo", yrepo.build())
.build().toString().getBytes(StandardCharsets.UTF_8)
)
).thenApply(
ignore -> new RsWithHeaders(
new RsWithStatus(RsStatus.FOUND),
new Headers.From("Location", String.format("/dashboard/%s/%s", user, name))
)
);
})
).thenApply(nothing -> name);
}).handle(
(name, throwable) -> {
final Response res;
if (throwable == null) {
res = new RsWithHeaders(
new RsWithStatus(RsStatus.FOUND),
new Headers.From("Location", String.format("/dashboard/%s/%s", user, name))
);
} else if (throwable.getCause() instanceof ArtipieException) {
res = new RsWithBody(
new RsWithStatus(RsStatus.BAD_REQUEST),
String.format("Invalid yaml input:\n%s", throwable.getCause().getMessage()),
StandardCharsets.UTF_8
);
} else {
res = new RsWithStatus(RsStatus.INTERNAL_ERROR);
}
return res;
}
)
);
}

Expand Down
41 changes: 41 additions & 0 deletions src/test/java/com/artipie/management/api/ApiRepoUpdateSliceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import com.artipie.asto.memory.InMemoryStorage;
import com.artipie.asto.test.ContentIs;
import com.artipie.http.Headers;
import com.artipie.http.hm.RsHasBody;
import com.artipie.http.hm.RsHasStatus;
import com.artipie.http.hm.SliceHasResponse;
import com.artipie.http.rq.RequestLine;
Expand All @@ -21,7 +22,9 @@
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import org.hamcrest.MatcherAssert;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.junit.jupiter.params.provider.ValueSource;
Expand Down Expand Up @@ -124,6 +127,44 @@ void updatesRepoConfiguration(final String extension, final boolean param) {
);
}

@Test
void returnsBabRequestOnInvalidYaml() {
MatcherAssert.assertThat(
"Returns BAD REQUEST",
new ApiRepoUpdateSlice(new FakeConfigFile(this.storage)),
new SliceHasResponse(
Matchers.allOf(
new RsHasStatus(RsStatus.BAD_REQUEST),
new RsHasBody("Invalid yaml input:\nRepo section is required".getBytes())
),
new RequestLine(
RqMethod.POST, String.format("/api/repos/%s", ApiRepoUpdateSliceTest.USER)
),
new Headers.From(
"Location", String.format("/dashboard/%s", ApiRepoUpdateSliceTest.userRepo())
),
new Content.From(body(ApiRepoUpdateSliceTest.REPO, "abc123 : sdfdsf : sdfsdf"))
)
);
}

Comment on lines +149 to +150
Copy link
Contributor

Choose a reason for hiding this comment

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

@olenagerasimova thanks, one suggestion. Probably to add negative test for INTERNAL_ERROR?

@Test
void returnsInternalErrorOnInvalidRequest() {
MatcherAssert.assertThat(
new ApiRepoUpdateSlice(new FakeConfigFile(this.storage)),
new SliceHasResponse(
new RsHasStatus(RsStatus.INTERNAL_ERROR),
new RequestLine(
RqMethod.POST, String.format("/api/repos/%s", ApiRepoUpdateSliceTest.USER)
),
new Headers.From(
"Location", String.format("/dashboard/%s", ApiRepoUpdateSliceTest.userRepo())
),
Content.EMPTY
)
);
}

private static byte[] body(final String reponame, final String yaml) {
return URLEncoder.encode(
String.format("repo=%s&config=%s", reponame, yaml),
Expand Down