Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature/fragment processing failure handling #468

Merged
merged 35 commits into from Dec 12, 2018
Merged

Feature/fragment processing failure handling #468

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
a07aa55
Fragment processing failure handling - initial commit
Oct 19, 2018
9c9ab3a
unneeded code removed
Oct 19, 2018
bad06f8
CR fixes
Oct 19, 2018
c24dd33
CR fixes
Oct 19, 2018
f2ee03c
Merge branch 'master' into feature/fragment-processing-failure-handling
tomaszmichalak Oct 19, 2018
66be8f9
Introduce Knot Routing to store per-service failure / processing info…
Oct 22, 2018
f8bba80
Merge branch 'feature/fragment-processing-failure-handling' of https:…
Oct 22, 2018
82113b1
fragment processing failure handling - CR fixes. Introduced KnotError…
Oct 24, 2018
40c677c
configurable fallback strategy with dedicated tag and ServiceLoader s…
Oct 26, 2018
3397a73
tests adjustment
Oct 26, 2018
8afb149
Merge branch 'master' into feature/fragment-processing-failure-handling
Oct 26, 2018
ab4e1ed
Redundant null check removed
Oct 26, 2018
63d2f4a
init knots property with stream (non-functional change)
Oct 26, 2018
8f50f38
import removed
Oct 26, 2018
b4e2b2d
CR fixes, run fallback matcher once
Oct 26, 2018
f31966f
allow BLANK fallback that requires no additional knotx:snippet element
Oct 29, 2018
3f3452a
fixed logger format
Oct 29, 2018
2b7fe85
Allow global fallback configuration through snippet options
Oct 30, 2018
48f43df
Cache fallback resolution output
Nov 5, 2018
72092b5
KnotStatus class renamed to KnotTaskStatus
Nov 5, 2018
e42f4c3
Store fallback fragment id in Fragment attributes
Nov 5, 2018
6872726
Save fallback strategy name in Fragment attributes object
Nov 5, 2018
2bd1551
Fallback fragment cache in request scope and other minor CR fixes
Nov 8, 2018
7045f8f
Fallback handling - documentation and changelog
Nov 8, 2018
903b79f
Fallback handling - documentation
Nov 8, 2018
b335a70
CR fixes, better test variables naming
Nov 8, 2018
bb67347
Update FallbackHandling.md
Skejven Nov 9, 2018
835dc05
Merge remote-tracking branch 'upstream/master' into feature/fragment-…
Dec 6, 2018
6cc045b
docs tweaks
scypio Dec 10, 2018
afbfc6e
reordered methods
scypio Dec 10, 2018
ff20f00
CR fix (javadoc)
scypio Dec 10, 2018
a3e3df6
moved fragment error handling to a separate method, throw FragmentExe…
scypio Dec 11, 2018
b5cc298
renamed exception to FragmentProcessingException
scypio Dec 11, 2018
3faba82
simplified global fallback definition
scypio Dec 11, 2018
ed5a5f1
fixed BLANK fallback template markup resolution
scypio Dec 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion knotx-core/src/main/java/io/knotx/assembler/FragmentAssemblerKnotProxyImpl.java
View file
Open in desktop
Expand Up @@ -16,6 +16,7 @@
package io.knotx.assembler;

import io.knotx.dataobjects.ClientResponse;
import io.knotx.dataobjects.Fragment;
import io.knotx.dataobjects.KnotContext;
import io.knotx.fragments.SnippetPatterns;
import io.knotx.knot.AbstractKnotProxy;
Expand Down Expand Up @@ -48,7 +49,7 @@ protected Single<KnotContext> processRequest(KnotContext knotContext) {
if (hasFragments(knotContext)) {
try {
String joinedFragments = knotContext.getFragments().stream()
.map(fragment -> options.getUnprocessedStrategy().get(fragment, patterns))
.map(this::processFragment)
.collect(Collectors.joining());

return Single.just(createSuccessResponse(knotContext, joinedFragments));
Expand All @@ -62,6 +63,11 @@ protected Single<KnotContext> processRequest(KnotContext knotContext) {
}
}

private String processFragment(Fragment fragment) {
return fragment.failed() && fragment.hasFallback() ? fragment.fallback()
tomaszmichalak marked this conversation as resolved.
Show resolved Hide resolved
: options.getUnprocessedStrategy().get(fragment, patterns);
}

private boolean hasFragments(KnotContext knotContext) {
return knotContext.getFragments() != null && !knotContext.getFragments().isEmpty();
}
Expand Down
67 changes: 62 additions & 5 deletions knotx-core/src/main/java/io/knotx/dataobjects/Fragment.java
View file
Open in desktop
Expand Up @@ -36,38 +36,56 @@ public class Fragment {
private static final String KNOTS_KEY = "_KNOTS";
private static final String CONTENT_KEY = "_CONTENT";
private static final String CONTEXT_KEY = "_CONTEXT";
private static final String FAILED_KEY = "_FAILED";
private static final String FAILURE_DETAILS_KEY = "_FAILEURE_DETAILS";
private static final String FALLBACK_KEY = "_FALLBACK";


private final List<String> knots;
private final JsonObject context;
private String content;
private boolean failed;
tomaszmichalak marked this conversation as resolved.
Show resolved Hide resolved
private String failureDetails;
Copy link
Member

Choose a reason for hiding this comment

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

I think about the solution that instead of failureDetails can provide more details about the flow. We could extend knots list with such details as:

  • knotIdentifier
  • processed: SUCCESS, FAILURE, UNPROCESSED
  • errors:
    • error code
    • error message

So if for example Data Bridge would not be able to call 2 services we would get:

  • knotIndetifier: dataBridge
  • processed: FAILURE
  • errors [ {error_from_service_1}, {error_from_service_2} ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changes implemented here, unfortunately github is not working that great right now so they are not visible in the PR: scypio@66be8f9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added Knot object to Fragment with knot name / status and errors information

private String fallback;

public Fragment(JsonObject fragment) {
this.knots = fragment.getJsonArray(KNOTS_KEY).stream().map(String::valueOf)
.collect(Collectors.toList());
this.content = fragment.getString(CONTENT_KEY);
this.context = fragment.getJsonObject(CONTEXT_KEY, new JsonObject());
this.failed = fragment.getBoolean(FAILED_KEY);
this.failureDetails = fragment.getString(FAILURE_DETAILS_KEY);
this.fallback = fragment.getString(FALLBACK_KEY);
}

private Fragment(List<String> knots, String data) {
private Fragment(List<String> knots, String data, String fallback) {
if (knots == null || knots.isEmpty() || StringUtils.isEmpty(data)) {
throw new NoSuchElementException("Fragment is not valid [" + knots + "], [" + data + "].");
}
this.knots = knots;
this.content = data;
this.context = new JsonObject();
this.fallback = fallback;
}

public static Fragment raw(String data) {
return new Fragment(Collections.singletonList(RAW_FRAGMENT_ID), data);
return new Fragment(Collections.singletonList(RAW_FRAGMENT_ID), data, null);
}

public static Fragment snippet(List<String> knots, String data, String fallback) {
return new Fragment(knots, data, fallback);
}

public static Fragment snippet(List<String> knots, String data) {
return new Fragment(knots, data);
return snippet(knots, data,null);
}

public JsonObject toJson() {
return new JsonObject().put(KNOTS_KEY, new JsonArray(knots)).put(CONTENT_KEY, content)
.put(CONTEXT_KEY, context);
.put(CONTEXT_KEY, context)
.put(FAILED_KEY, failed)
.put(FAILURE_DETAILS_KEY, failureDetails)
.put(FALLBACK_KEY, fallback);
}

/**
Expand Down Expand Up @@ -105,6 +123,42 @@ public boolean isRaw() {
return knots.contains(RAW_FRAGMENT_ID);
}

/**
* @return true if processing of this Fragment has failed
*/
public boolean failed() {
return failed;
}

/**
* @return replacement markup that should be rendered if this Fragment has failed. Can be empty
* string. Null value indicates that no replacement markup is provided.
Copy link
Member

Choose a reason for hiding this comment

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

Could you please fix javadoc, this never should be null. You probably ment:

absent value indicates that no replacement markup is provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

*/
public String fallback() {
tomaszmichalak marked this conversation as resolved.
Show resolved Hide resolved
return fallback;
}

/**
* @return failure details for this Fragment
*/
public String failureDetails() {
return failureDetails;
}

public Fragment failed(boolean failed) {
this.failed = failed;
tomaszmichalak marked this conversation as resolved.
Show resolved Hide resolved
return this;
}

public Fragment failureDetails(String failureDetails) {
this.failureDetails = failureDetails;
return this;
}

public boolean hasFallback() {
return fallback != null;
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand All @@ -116,7 +170,10 @@ public boolean equals(Object o) {
Fragment that = (Fragment) o;
return Objects.equal(knots, that.knots) &&
Objects.equal(content, that.content) &&
Objects.equal(context, that.context);
Objects.equal(context, that.context) &&
Objects.equal(failed, that.failed) &&
Objects.equal(failureDetails, that.failureDetails) &&
Objects.equal(fallback, that.fallback);
}

@Override
Expand Down
2 changes: 2 additions & 0 deletions knotx-core/src/main/java/io/knotx/fragments/FragmentConstants.java
View file
Open in desktop
Expand Up @@ -19,6 +19,8 @@ public final class FragmentConstants {

public static final String SNIPPET_IDENTIFIER_NAME = "knots";

public static final String SNIPPET_FALLBACK_NAME = "fallback";

public static final String FRAGMENT_IDENTIFIERS_SEPARATOR = ",";

static final int DEBUG_MAX_FRAGMENT_CONTENT_LOG_LENGTH = 256;
Expand Down
13 changes: 13 additions & 0 deletions knotx-core/src/main/java/io/knotx/fragments/SnippetPatterns.java
View file
Open in desktop
Expand Up @@ -26,9 +26,14 @@ public class SnippetPatterns {
private static final String SNIPPET_PATTERN =
"<%s\\s+%s" + FragmentConstants.SNIPPET_IDENTIFIER_NAME
+ "\\s*=\\s*\"([A-Za-z0-9-,]+)\"[^>]*>.+?</%s>";
private static final String SNIPPET_WITH_FALLBACK_PATTERN =
tomaszmichalak marked this conversation as resolved.
Show resolved Hide resolved
"<%s\\s+%s" + FragmentConstants.SNIPPET_IDENTIFIER_NAME
+ "\\s*=\\s*\"([A-Za-z0-9-,]+)\"[^>]*"
Copy link
Member

Choose a reason for hiding this comment

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

I think some whitespace is missed here ;) Do we have unit tests for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have unit tests and they work, but this part will be rewritten anyway

+"%s"+ FragmentConstants.SNIPPET_FALLBACK_NAME + "\\s*=\\s*\"([^\"]*)\"[^>]*>.+?</%s>";

private final Pattern anySnippetPattern;
private final Pattern snippetPattern;
private final Pattern snippetWithFallbackPattern;

public SnippetPatterns(SnippetOptions snippetOptions) {
anySnippetPattern = Pattern
Expand All @@ -38,6 +43,10 @@ public SnippetPatterns(SnippetOptions snippetOptions) {
.compile(String
.format(SNIPPET_PATTERN, snippetOptions.getTagName(), snippetOptions.getParamsPrefix(),
snippetOptions.getTagName()), Pattern.DOTALL);
snippetWithFallbackPattern = Pattern
.compile(String
.format(SNIPPET_WITH_FALLBACK_PATTERN, snippetOptions.getTagName(), snippetOptions.getParamsPrefix(),
snippetOptions.getParamsPrefix(), snippetOptions.getTagName()), Pattern.DOTALL);
}

public Pattern getAnySnippetPattern() {
Expand All @@ -47,4 +56,8 @@ public Pattern getAnySnippetPattern() {
public Pattern getSnippetPattern() {
return snippetPattern;
}

public Pattern getSnippetWithFallbackPattern() { return snippetWithFallbackPattern; }

}

11 changes: 11 additions & 0 deletions knotx-core/src/main/java/io/knotx/knot/AbstractKnotProxy.java
View file
Open in desktop
Expand Up @@ -15,6 +15,7 @@
*/
package io.knotx.knot;

import com.google.common.collect.Sets;
import io.knotx.dataobjects.Fragment;
import io.knotx.dataobjects.KnotContext;
import io.knotx.proxy.KnotProxy;
Expand Down Expand Up @@ -72,6 +73,16 @@ public void process(KnotContext knotContext, Handler<AsyncResult<KnotContext>> r
/**
* Method lets you decide whether the Fragment should be processed by your Knot or not.
*
* @param fragment Fragment to process - or skip {@link Fragment}.
* @return <tt>true</tt> if this Knot should process current {@link Fragment}.
*/
protected boolean shouldProcess(Fragment fragment) {
Copy link
Member

Choose a reason for hiding this comment

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

I can not see that this method is invoked in AbstractKnotProxy. It should be a part of default behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a tricky one, shouldProcess was accepting only Set beforehand.
protected abstract Single<KnotContext> processRequest(KnotContext knotContext);

We should mark that method as deprecated and go with

protected boolean shouldProcess(Set<String> knots, Fragment fragment) {
  // default implementation
}

Copy link
Member

Choose a reason for hiding this comment

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

Let's mark it @Deprecated then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

new task created to tackle this: #474

Copy link
Member

Choose a reason for hiding this comment

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

Let's mark shouldProcess(Set<String> knots) as @Deprecated. Method you suggested should replace it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved here: #474

return fragment != null && !fragment.failed() && shouldProcess(Sets.newHashSet(fragment.knots()));
Copy link
Member

Choose a reason for hiding this comment

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

I think fragment can not be null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

redundant check removed

}

/**
* Method lets you decide whether the Fragment with a given set of Knots should be processed by your Knot or not
*
* @param knots set of all Knots names that occurred in the current {@link KnotContext}.
* @return <tt>true</tt> if this Knot should process current {@link KnotContext}.
*/
Expand Down
7 changes: 6 additions & 1 deletion knotx-core/src/main/java/io/knotx/splitter/HtmlFragmentSplitter.java
View file
Open in desktop
Expand Up @@ -15,6 +15,8 @@
*/
package io.knotx.splitter;

import static org.jsoup.parser.Parser.unescapeEntities;

import com.google.common.collect.Lists;
import io.knotx.dataobjects.Fragment;
import io.knotx.fragments.FragmentConstants;
Expand Down Expand Up @@ -64,6 +66,9 @@ private Fragment toRaw(String html, int startIdx, int endIdx) {
}

private Fragment toSnippet(String[] ids, String html, int startIdx, int endIdx) {
return Fragment.snippet(Arrays.asList(ids), html.substring(startIdx, endIdx));
Matcher matcher = snippetPatterns.getSnippetWithFallbackPattern().matcher(html);
String fallback = matcher.matches() ? unescapeEntities(matcher.group(2), true) : null;
return Fragment.snippet(Arrays.asList(ids), html.substring(startIdx, endIdx), fallback);
}

}
53 changes: 37 additions & 16 deletions knotx-core/src/test/java/io/knotx/assembler/FragmentAssemblerTest.java
View file
Open in desktop
Expand Up @@ -34,7 +34,9 @@
import java.util.List;
import org.apache.commons.lang3.StringUtils;
import org.apache.commons.lang3.tuple.ImmutablePair;
import org.apache.commons.lang3.tuple.ImmutableTriple;
import org.apache.commons.lang3.tuple.Pair;
import org.apache.commons.lang3.tuple.Triple;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
Expand Down Expand Up @@ -64,7 +66,7 @@ public void callAssemblerWithEmptySnippet_expectNoContentStatus(
VertxTestContext context, Vertx vertx) {
callAssemblerWithAssertions(context, vertx,
Collections
.singletonList(new ImmutablePair<>(Collections.singletonList(RAW), StringUtils.SPACE)),
.singletonList(new ImmutableTriple<>(Collections.singletonList(RAW), null, StringUtils.SPACE)),
knotContext -> Assertions.assertEquals(HttpResponseStatus.NO_CONTENT.code(),
knotContext.getClientResponse().getStatusCode()));
}
Expand All @@ -73,10 +75,10 @@ public void callAssemblerWithEmptySnippet_expectNoContentStatus(
@KnotxApplyConfiguration("io/knotx/assembler/test.asIs.io.knotx.FragmentAssembler.json")
public void callAssemblerWithManySnippets_expectAsIsResult(
VertxTestContext context, Vertx vertx) throws IOException {
List<Pair<List<String>, String>> fragments = Arrays.asList(
toPair("io/knotx/assembler/fragment1.txt", RAW),
toPair("io/knotx/assembler/fragment2.txt", SERVICES, HANDLEBARS),
toPair("io/knotx/assembler/fragment3.txt", RAW));
List<Triple<List<String>, String, String>> fragments = Arrays.asList(
toTriple("io/knotx/assembler/fragment1.txt",null, RAW),
toTriple("io/knotx/assembler/fragment2.txt", null, SERVICES, HANDLEBARS),
toTriple("io/knotx/assembler/fragment3.txt", null, RAW));
String expectedResult = FileReader.readText("io/knotx/server/expectedAsIsResult.html");
callAssemblerWithAssertions(context, vertx,
fragments,
Expand All @@ -92,10 +94,10 @@ public void callAssemblerWithManySnippets_expectAsIsResult(
@KnotxApplyConfiguration("io/knotx/assembler/test.unwrap.io.knotx.FragmentAssembler.json")
public void callAssemblerWithManySnippets_expectUnwrapResult(
VertxTestContext context, Vertx vertx) throws IOException {
List<Pair<List<String>, String>> fragments = Arrays.asList(
toPair("io/knotx/assembler/fragment1.txt", RAW),
toPair("io/knotx/assembler/fragment2.txt", SERVICES, HANDLEBARS),
toPair("io/knotx/assembler/fragment3.txt", RAW));
List<Triple<List<String>, String, String>> fragments = Arrays.asList(
toTriple("io/knotx/assembler/fragment1.txt", null, RAW),
toTriple("io/knotx/assembler/fragment2.txt", null, SERVICES, HANDLEBARS),
toTriple("io/knotx/assembler/fragment3.txt", null, RAW));
String expectedResult = FileReader.readText("io/knotx/server/expectedUnwrapResult.html");
callAssemblerWithAssertions(context, vertx,
fragments,
Expand All @@ -111,10 +113,10 @@ public void callAssemblerWithManySnippets_expectUnwrapResult(
@KnotxApplyConfiguration("io/knotx/assembler/test.ignore.io.knotx.FragmentAssembler.json")
public void callAssemblerWithManySnippets_expectIgnoreResult(
VertxTestContext context, Vertx vertx) throws IOException {
List<Pair<List<String>, String>> fragments = Arrays.asList(
toPair("io/knotx/assembler/fragment1.txt", RAW),
toPair("io/knotx/assembler/fragment2.txt", SERVICES, HANDLEBARS),
toPair("io/knotx/assembler/fragment3.txt", RAW));
List<Triple<List<String>,String, String>> fragments = Arrays.asList(
toTriple("io/knotx/assembler/fragment1.txt", null, RAW),
toTriple("io/knotx/assembler/fragment2.txt", null, SERVICES, HANDLEBARS),
toTriple("io/knotx/assembler/fragment3.txt", null, RAW));
String expectedResult = FileReader.readText("io/knotx/server/expectedIgnoreResult.html");
callAssemblerWithAssertions(context, vertx,
fragments,
Expand All @@ -126,9 +128,28 @@ public void callAssemblerWithManySnippets_expectIgnoreResult(
});
}

@Test
@KnotxApplyConfiguration("io/knotx/assembler/test.asIs.io.knotx.FragmentAssembler.json")
public void callAssemblerWithFailedSnippet_expectFallback(
VertxTestContext context, Vertx vertx) throws IOException {
List<Triple<List<String>,String, String>> fragments = Arrays.asList(
toTriple("io/knotx/assembler/fragment1.txt", null, RAW),
toTriple("io/knotx/assembler/fragment2.txt", "<p class='error'>fallback</p>\n", SERVICES, HANDLEBARS),
toTriple("io/knotx/assembler/fragment3.txt", null, RAW));
String expectedResult = FileReader.readText("io/knotx/server/expectedFallbackResult.html");
callAssemblerWithAssertions(context, vertx,
fragments,
knotContext -> {
Assertions.assertEquals(HttpResponseStatus.OK.code(),
knotContext.getClientResponse().getStatusCode());
Assertions
.assertEquals(expectedResult, knotContext.getClientResponse().getBody().toString());
});
}

private void callAssemblerWithAssertions(
VertxTestContext context, Vertx vertx,
List<Pair<List<String>, String>> fragments,
List<Triple<List<String>, String, String>> fragments,
Consumer<KnotContext> verifyResultFunction) {
KnotProxy service = KnotProxy.createProxy(vertx, ADDRESS);

Expand All @@ -137,8 +158,8 @@ private void callAssemblerWithAssertions(
subscribeToResult_shouldSucceed(context, knotContextSingle, verifyResultFunction);
}

private Pair<List<String>, String> toPair(String filePath, String... knots) throws IOException {
return new ImmutablePair<>(Arrays.asList(knots), FileReader.readText(filePath));
private Triple<List<String>, String, String> toTriple(String filePath, String failed, String... knots) throws IOException {
return new ImmutableTriple<>(Arrays.asList(knots), failed, FileReader.readText(filePath));
}

}
5 changes: 3 additions & 2 deletions knotx-core/src/test/java/io/knotx/junit/util/KnotContextFactory.java
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@
import java.util.stream.Collectors;
import org.apache.commons.lang3.StringUtils;
import org.apache.commons.lang3.tuple.Pair;
import org.apache.commons.lang3.tuple.Triple;

public final class KnotContextFactory {

Expand All @@ -51,11 +52,11 @@ public static KnotContext empty(String template) {
.setClientRequest(new ClientRequest());
}

public static KnotContext create(List<Pair<List<String>, String>> fragments) {
public static KnotContext create(List<Triple<List<String>, String, String>> fragments) {
return new KnotContext()
.setFragments(
fragments != null
? fragments.stream().map(data -> Fragment.snippet(data.getKey(), data.getValue())).collect(Collectors.toList())
? fragments.stream().map(data -> Fragment.snippet(data.getLeft(), data.getRight(), data.getMiddle()).failed(data.getMiddle() != null)).collect(Collectors.toList())
: null)
.setClientRequest(new ClientRequest())
.setClientResponse(
Expand Down