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.

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

Feature/fragment processing failure handling #468

Merged
merged 35 commits into from Dec 12, 2018

Conversation

Projects
3 participants
@scypio
Copy link
Contributor

scypio commented Oct 19, 2018

IMPLEMENTATION IN PROGRESS

Description

Implementation of #466

Motivation and Context

Screenshots (if appropriate)

Upgrade notes (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

I hereby agree to the terms of the Knot.x Contributor License Agreement.

@tomaszmichalak

This comment has been minimized.

Knots should not be able to modify the fallback. It is restricted to snippet.

This comment has been minimized.

Copy link
Owner

scypio replied Oct 19, 2018

fixed

Jakub Berlinski added some commits Oct 19, 2018

Jakub Berlinski
Jakub Berlinski

private final List<String> knots;
private final JsonObject context;
private String content;
private boolean failed;
private String failureDetails;

This comment has been minimized.

@tomaszmichalak

tomaszmichalak Oct 19, 2018

Contributor

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} ]

This comment has been minimized.

@scypio

scypio Oct 22, 2018

Contributor

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

This comment has been minimized.

@scypio

scypio Oct 22, 2018

Contributor

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

tomaszmichalak and others added some commits Oct 19, 2018

@scypio scypio changed the base branch from master to feature/vertx-3_4_2-update Oct 22, 2018

@scypio scypio changed the base branch from feature/vertx-3_4_2-update to master Oct 23, 2018

Show resolved Hide resolved ...ore/src/main/java/io/knotx/assembler/FragmentAssemblerKnotProxyImpl.java Outdated
Show resolved Hide resolved knotx-core/src/main/java/io/knotx/dataobjects/Fragment.java Outdated
Show resolved Hide resolved knotx-core/src/main/java/io/knotx/dataobjects/Fragment.java Outdated
Show resolved Hide resolved knotx-core/src/main/java/io/knotx/dataobjects/Fragment.java Outdated
Show resolved Hide resolved knotx-core/src/main/java/io/knotx/dataobjects/Knot.java Outdated
Show resolved Hide resolved knotx-core/src/main/java/io/knotx/fragments/SnippetPatterns.java
* @return <tt>true</tt> if this Knot should process current {@link Fragment}.
*/
protected boolean shouldProcess(Fragment fragment) {
return fragment != null && !fragment.failed() && shouldProcess(Sets.newHashSet(fragment.knots()));

This comment has been minimized.

@tomaszmichalak

tomaszmichalak Oct 23, 2018

Contributor

I think fragment can not be null.

This comment has been minimized.

@scypio

scypio Oct 26, 2018

Contributor

redundant check removed

* @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) {

This comment has been minimized.

@tomaszmichalak

tomaszmichalak Oct 23, 2018

Contributor

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

This comment has been minimized.

@scypio

scypio Oct 24, 2018

Contributor

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
}

This comment has been minimized.

@Skejven

Skejven Nov 9, 2018

Contributor

Let's mark it @Deprecated then.

This comment has been minimized.

@scypio

scypio Dec 11, 2018

Contributor

new task created to tackle this: #474

Show resolved Hide resolved knotx-core/src/main/java/io/knotx/util/FragmentUtil.java Outdated
return fragment;
}

public static Fragment success(Fragment fragment, String knot) {

This comment has been minimized.

@tomaszmichalak

tomaszmichalak Oct 23, 2018

Contributor

It should be method in Fragment.

This comment has been minimized.

@scypio

scypio Oct 24, 2018

Contributor

moved to Fragment

Jakub Berlinski
fragment processing failure handling - CR fixes. Introduced KnotError…
… class, adjusted Fragment, removed FragmentUtil
this.knots = fragment.getJsonArray(KNOTS_KEY).stream().map(String::valueOf)
.collect(Collectors.toList());
JsonArray knotsArray = fragment.getJsonArray(KNOTS_KEY);
this.knots = new ArrayList<>();

This comment has been minimized.

@Skejven

Skejven Oct 24, 2018

Contributor

How about:

Suggested change Beta
this.knots = new ArrayList<>();
this.knots= knotsArray.stream()
.map(entry -> new Knot((JsonObject) entry))
.collect(Collectors.toList());

If you don't like doing this streams way, at least please initialize list with size new ArrayList<>(knotsArray.size());

This comment has been minimized.

@scypio

scypio Oct 26, 2018

Contributor

I like it, changed.

import java.util.List;

@DataObject(inheritConverter = true)
public class Knot {

This comment has been minimized.

@Skejven

Skejven Oct 26, 2018

Contributor

Correct me if I understood this wrong, but this class looks to me more like KnotTask or KnotJob (or smth. like that).
This is not Knot and naming it like that will introduce a lot of confusion. See here: https://github.com/Cognifide/knotx/wiki/Knot what Knot is.

If you will apply rename change here, please also think about KnotStatus and KnotError classes.

This comment has been minimized.

@scypio

scypio Oct 26, 2018

Contributor

renamed to KnotTask

public Pattern getAnySnippetPattern() {
return anySnippetPattern;
}
public Pattern getAnySnippetPattern() { return anySnippetPattern; }

This comment has been minimized.

@Skejven

Skejven Oct 26, 2018

Contributor

Please make it 3-liner again (as the methods below).
Now it is inconsistent with google formatting rules for java that we follow (see more in contributing guide)

This comment has been minimized.

@scypio

scypio Oct 26, 2018

Contributor

file reformatted

@@ -35,14 +37,17 @@

@Override
public List<Fragment> split(String html) {
if (StringUtils.isEmpty(html)) {
throw new NoSuchElementException("html cannot be empty");
}
List<Fragment> fragments = Lists.newLinkedList();

This comment has been minimized.

@Skejven

Skejven Oct 26, 2018

Contributor

I know that this is not part of the change you introduced, but could you change this to ArrayList here?
Thank you.

This comment has been minimized.

@scypio

scypio Oct 26, 2018

Contributor

done

}
return fragments;
}

private Fragment toRaw(String html, int startIdx, int endIdx) {
return Fragment.raw(html.substring(startIdx, endIdx));
private void processMarkup(List<Fragment> fragments, String data, int startIdx, int endIdx) {

This comment has been minimized.

@Skejven

Skejven Oct 26, 2018

Contributor

Now, for each found fragment, searching for fallbacks is triggered and searching for matching fallback is done from the beginning. Wouldn't be more efficient, to build some cache/map of fallbacks first and then only add proper fallback (if exists) by name?

This comment has been minimized.

@scypio

scypio Oct 26, 2018

Contributor

Introduced FallbackMarkers to run the fallback matcher once against the whole html

@scypio

This comment has been minimized.

Copy link
Contributor

scypio commented Oct 30, 2018

Features implemented so far:

1. Simple blank fallback

just add knotx-data-fallback="BLANK" to your snippet:

<knotx-snippet data-knotx-knots="databridge,handlebars" 
  data-knotx-databridge-name="unstable-service"
  data-knotx-fallback="BLANK">
      {{#if _result}}<h2>{{_result.count}}</h2>{{/if}}
</knotx-snippet>

2. Static markup fallback

create a fallback snippet with data-knotx-fallback-id attribute and point to it in your snippet:

<knotx-snippet data-knotx-knots="databridge,handlebars" 
  data-knotx-databridge-name="unstable-service"
  data-knotx-fallback="CUSTOM">
      {{#if _result}}<h2>{{_result.count}}</h2>{{/if}}
</knotx-snippet>
<knotx:fallback data-knotx-fallback-id="CUSTOM">
  <p class="error">error</p>
</knotx:fallback>

3. Global blank fallback

to apply BLANK fallback to all snippets by default configure defaultFallback attribute within snippetOptions - apply these to both Splitter and FragmentAssembler.

  snippetOptions {
    tagName = knotx-snippet
    paramsPrefix = data-knotx-
    defaultFallback = BLANK
  }

4. Global fallback with custom markup

to apply custom markup fallback to all snippets by default:

  • configure defaultFallback attribute within snippetOptions
  • define your own fallbacks property within snippetOptions
  • apply these to both Splitter and FragmentAssembler.
  snippetOptions {
    tagName = knotx-snippet
    paramsPrefix = data-knotx-
    defaultFallback = CUSTOM_GLOBAL
    fallbacks = [
      {
        id = CUSTOM_GLOBAL
        markup = "<knotx:fallback data-knotx-fallback-id='CUSTOM_GLOBAL'><p class="error">error</p></knotx:fallback>"
      }
    ]
  }

5. fallback with custom FallbackStrategy

you can deliver your own class that will be fired by the assembler if given fragment fails

  • implement FallbackStrategy interface
import io.knotx.dataobjects.Fragment;
import io.knotx.dataobjects.KnotContext;
import io.knotx.fallback.FallbackStrategy;

public class MyFallbackStrategy implements FallbackStrategy {

  @Override
  public String getId() {
    return "CUSTOM_STRATEGY";
  }

  @Override
  public String applyFallback(Fragment failed, Fragment fallback, KnotContext knotContext) {
    return "<p>markup</p>";
  }
}
  • make sure that ServiceLoader can discover this class - add its fully qualified name to /META-INF/services/io.knotx.fallback.FallbackStrategy file.
  • create a custom fallback snippet with linked strategy Id
<knotx-snippet data-knotx-knots="databridge,handlebars" 
  data-knotx-databridge-name="unstable-service"
  data-knotx-fallback="CUSTOM">
      {{#if _result}}<h2>{{_result.count}}</h2>{{/if}}
</knotx-snippet>
<knotx:fallback data-knotx-fallback-id="CUSTOM" data-knotx-fallback-strategy="CUSTOM_STRATEGY">
  <p class="error">error</p>
</knotx:fallback>
@tomaszmichalak

This comment has been minimized.

Copy link
Contributor

tomaszmichalak commented Oct 31, 2018

I am wondering if this tag data-knotx-fallback="BLANK" is really required. We can assume that if there is no data-knotx-fallback-id tag defined, then the default fallback mechanism should handle the snippet.

@tomaszmichalak

This comment has been minimized.

Copy link
Contributor

tomaszmichalak commented Oct 31, 2018

According to 5:
We agreed that a fallback strategy should be configured as a chain, something like:

"fallbackStrategies": [
   { "name": "CUSTOM", conf":"{next:"DEFAULT"" }},
   { "name": "DEFAULT", "conf":"EMPTY" }
]

In that way our fallback strategy implementation should pass the flow to the next fallback. I thought about something similar to io.knotx.server.handler.knot.KnotEngineRoutingHandlerFactory.KnotxEngineHandler#handleRoute.

@tomaszmichalak

This comment has been minimized.

Copy link
Contributor

tomaszmichalak commented Oct 31, 2018

Can we move this comment to the SnippetFallback.md file - we could work on documentation in the same PR. We plan to move documentation from wiki to *.md files so it can be good option.

*/
package io.knotx.dataobjects;

public enum KnotStatus {

This comment has been minimized.

@Skejven

Skejven Oct 31, 2018

Contributor

Please see my previous comments about renaming KnotTask related classes.
E.g. this enum does not define status of Knot. It defines status of the KnotTask.

This comment has been minimized.

@scypio

scypio Oct 31, 2018

Contributor

will rename this class as well

This comment has been minimized.

@scypio

scypio Nov 5, 2018

Contributor

class renamed

}
}

public String applyFallback(Fragment failed, KnotContext knotContext) {

This comment has been minimized.

@Skejven

Skejven Oct 31, 2018

Contributor

@scypio in my previous comment I suggested building cache for fallbacks ( fallbackId -> fallback Fragment).

IMO from the performance perspective current implementation of FragmentAssemblerFallbackHandler is not acceptable. That's why:

  • for the each failed fragment (might be 0, might be 1 might be 100 on a single page) whole markup tree (fragments list) is traversed
  • in order to find if the checked fragment is a fallback, besides checking isFragment flag, markup operation needs to be applied on the piece of markup (using Jsoup) in order to check an attribute that contains fallback name (this is expensive!)
  • if the fallback is found (by its ID) its content is returned
  • if the fallback is not found in the whole markup, the configuration is traversed then.

And... If there is a next fragment that failed on the page, the whole traversing and searching for fallback is done once again.
I'd suggest again building a cache (preferably lazy-initialized, for the moment in this handler) that keeps fallbackId -> fallback<Fragment>.

Proposition for the future
In the future that cache might be event saved outside the request scope (I believe that it wouldn't be a very common situation to change dynamically fallback for a snippet). There is a similar cache implementation in the Handlebars Knot. It holds compiled by Handlebars fragment by its hash.

This comment has been minimized.

@scypio

scypio Oct 31, 2018

Contributor

will implement lazy-initialized cache

This comment has been minimized.

@scypio

scypio Nov 5, 2018

Contributor

cache added to FragmentAssemblerFallbackHandler

} else if (idx < fe.start) {
fragments.add(Fragment.raw(html.substring(idx, fe.start)));
}
fragments.add(toFallback(html, fe.start, fe.end));

This comment has been minimized.

@Skejven

Skejven Oct 31, 2018

Contributor

Maybe it would be worth to keep additional field in the Fragment e.g. fragmentId and extract it here?
That will allow to not use markup processing (JSoup) at the Assembler level to extract fallback id.

This comment has been minimized.

@scypio

scypio Oct 31, 2018

Contributor

I like this idea
data-knotx-fragment-id - should hold id of current fragment (new parameter)
data-knotx-fallback - should hold id of a other (fallback) fragment

This comment has been minimized.

@scypio

scypio Nov 5, 2018

Contributor

I'm having some second thoughts on this, it would require additional capture groups in the regexps. I'm afraid we would take a performance hit here.

This comment has been minimized.

@scypio

scypio Nov 8, 2018

Contributor

Finally I decided to remove JSoup parsing from the Assembler.
Additional regex to extract the fragment id is run only against fallback snippets.

@Skejven

This comment has been minimized.

Copy link
Contributor

Skejven commented Nov 2, 2018

Please see my #466 (comment). I described there few implementation details that will make this solution easier to extend in the future.


private final Map<String, FallbackStrategy> fallbackStrategies = Maps.newHashMap();

private final Map<String, Fragment> fallbackFragmentCache = Maps.newHashMap();

This comment has been minimized.

@Skejven

Skejven Nov 6, 2018

Contributor

It looks that this cache is not in request scope. Notice, that FragmentAssemblerFallbackHandler is instantiated once per FragmentAssemblerVerticle (in the FragmentAssemblerKnotProxyImpl). That means - each request processed by Knot.x will add its fallback to this cache. There are few scenarios when something can go really wrong with this apporach:

  • (less likely) in pessimistic case, when there is huge amount of fallbacks, this map can be really big,
  • (more likely) when Knot.x instance is not restarted for a long time, but there are many fallback with dynamic names that changes frequently (e.g. because of the authoring that enables changing fallback name in the component), this map can grow big,
  • (most likely) when fallback id will not change but it contents will, the old value taken from the cache by the same key will be outdated.

I can see 2 solutions here:

  1. Simple: thanks to that there are no longer jSoup here and reading fallback is much quicker now, you may remove the cache
  2. Advanced: Look at the implementation of a cache in the Handlebars Knot. It works beyond the request scope, but there are mechanics that handle all the cases I mentioned above.

This comment has been minimized.

@scypio

scypio Nov 8, 2018

Contributor

I just pass new HashMap instance for each incoming request, I think this should do the trick?

private static final String MESSAGE_KEY = "_MESSAGE";

private String code;
private Object message;

This comment has been minimized.

@Skejven

Skejven Nov 6, 2018

Contributor

What is the reason for message to be the Object not String?

This comment has been minimized.

@scypio

scypio Nov 8, 2018

Contributor

I've had a request to make it an Object to have the ability to store structured data. Opting to keep it this way


@DataObject(inheritConverter = true)
public class KnotTask {
private String name;

This comment has been minimized.

@Skejven

Skejven Nov 6, 2018

Contributor

Shouldn't name be final?

This comment has been minimized.

@scypio

scypio Nov 8, 2018

Contributor

Changed to final

.orElse(DefaultFallbackStrategy.ID);
return Optional.ofNullable(fallbackStrategies.get(strategyId)).orElseThrow(() -> {
LOGGER.error("Fragment {} specifies fallback strategy but no fallback strategy with given id was found", fallbackFragment);
return new IllegalStateException(String.format("no strategy with id %s found", strategyId));

This comment has been minimized.

@Skejven

Skejven Nov 6, 2018

Contributor

This is just cosmetics, but I'm not sure this is proper exception used here.
I see IllegalStateException as the wrong time to do some logic. Maybe IllegalArgumentException would be better fit here, what do you think?

This comment has been minimized.

@scypio

scypio Nov 8, 2018

Contributor

I agree, changed to IllegalArgumentException

private static final String DEFAULT_PARAMS_PREFIX = "data-knotx-";
private List<FallbackMetadata> DEFAULT_FALLBACKS = Lists.newArrayList(new FallbackMetadata("BLANK", "<knotx:fallback data-knotx-fallback-id=\"BLANK\"></knotx:fallback>"));

This comment has been minimized.

@Skejven

Skejven Nov 6, 2018

Contributor

Shouldn't the default content be:
new FallbackMetadata("BLANK", "<"+tagName+":fallback "+paramsPrefix+"fallback-id=\"BLANK\"></"+tagName+":fallback>")
?

Also, cosmetics: please fix the name of this field - it is not declared as a constant but has uppercase name.

This comment has been minimized.

@scypio

scypio Nov 8, 2018

Contributor

Method adjusted

Jakub Berlinski and others added some commits Nov 8, 2018

Jakub Berlinski
@Skejven
Copy link
Contributor

Skejven left a comment

Really good docs, thank you.

This code looks good to me.
Could you please make following changes in order to introduce this functionality to the whole Knot.x:

  • update Knot.x stack with commented out snippetOptions that defines defaultFallback = BLANK
  • update Knot.x example project with an example that shows how fallback works, e.g.:
    • BLANK defined as a global strategy
    • One custom knotx:fallback snippet
    • Example page, with 3 snippets, where one works properly, 1 fails to the BLANK strategy and one fails with custom fallback snippet.
data-knotx-fallback="CUSTOM">
{{#if _result}}<h2>{{_result.count}}</h2>{{/if}}
</knotx-snippet>
<knotx:fallback data-knotx-fallback-id="CUSTOM">

This comment has been minimized.

@Skejven

Skejven Nov 9, 2018

Contributor

What do you think about changing CUSTOM to e.g. MY_FALLBACK_ID.
That might be more clear, that CUSTOM is not the mandatory id part for the custom fallback snippet.

This comment has been minimized.

@scypio

scypio Dec 10, 2018

Contributor

Changed

This comment has been minimized.

@scypio

scypio Dec 10, 2018

Contributor

also: knotx-stack and knotx-example-project have PRs open that showcase this feature.

* @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) {

This comment has been minimized.

@Skejven

Skejven Nov 9, 2018

Contributor

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

This comment has been minimized.

@scypio

scypio Dec 11, 2018

Contributor

moved here: #474

@Skejven Skejven added this to In progress in Knot.x 1.5 Dec 5, 2018

Jakub Berlinski and others added some commits Dec 6, 2018


/**
* @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.

This comment has been minimized.

@Skejven

Skejven Dec 10, 2018

Contributor

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

absent value indicates that no replacement markup is provided.

This comment has been minimized.

@scypio

scypio Dec 10, 2018

Contributor

Fixed

scypio added some commits Dec 10, 2018

Knot.x 1.5 automation moved this from In progress to Reviewer approved Dec 12, 2018


public FragmentAssemblerFallbackHandler(FragmentAssemblerOptions options) {
this.options = options;
ServiceLoader<FallbackStrategy> fallbackStrategyServiceLoader = ServiceLoader.load(FallbackStrategy.class);

This comment has been minimized.

@tomaszmichalak

tomaszmichalak Dec 12, 2018

Contributor

Please extract it to a separate method.

This comment has been minimized.

@Skejven

Skejven Dec 12, 2018

Contributor

Agreed to do this later.

@Skejven Skejven merged commit eb3e620 into Cognifide:master Dec 12, 2018

2 checks passed

CodeFactor No issues found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Knot.x 1.5 automation moved this from Reviewer approved to Done Dec 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment