added examples #374

VivianLopes · 2019-01-29T09:29:47Z

No description provided.

kvosper · 2019-01-29T11:14:51Z

plugin-examples/src/main/java/com/hotels/styx/ExampleMessageIntercept.java

@@ -0,0 +1,78 @@
+package com.hotels.styx;


This is not a valid Java class. I will explain why further down. To avoid submitting invalid classes in pull requests, please attempt to run the associated unit test(s) first.

kvosper · 2019-01-29T11:19:09Z

plugin-examples/src/main/java/com/hotels/styx/ExampleMessageIntercept.java

+ * This shows an example of an inteceptor that responds to a received request
+ */
+
+public class Example implements Plugin {


Java class names must match the file name. I would not choose either of Example (too vague) or ExampleMessageIntercept (unclear, not ending with a noun).

Perhaps EarlyReturnExamplePlugin would be a good name.

Another thing to note is that there is already a class called ExamplePlugin. That was fine when there was only one example plugin, but now that you have added another, it has become too vague. You should come up with a new name and rename it.

kvosper · 2019-01-29T11:23:14Z

plugin-examples/src/main/java/com/hotels/styx/ExampleMessageIntercept.java

+    @Override
+    public Eventual<LiveHttpResponse> intercept(LiveHttpRequest request, Chain chain) {
+        if (request.header("X-Respond").isPresent()) {
+            // request.consume();


There should not be any commented code here.

Also please add a comment or two explaining what this code is doing - e.g. explain the branching condition and what happens in each branch.

kvosper · 2019-01-29T11:28:05Z

plugin-examples/src/main/java/com/hotels/styx/ExampleMessageIntercept.java

+
+//Example:
+
+chain.proceed(request)


We have dangling Java code here that is outside of any valid part of the file. This will prevent it from compiling. Presumably it is meant to be commented too.

However, it doesn't really make sense to have all these comments/code examples underneath the class file. If we have one example as a proper class with accompanying test, shouldn't the rest of the examples be done in the same way?

kvosper · 2019-01-29T11:29:32Z

plugin-examples/src/test/java/com/hotels/styx/ExampleMessageInterceptTest.java

+ * This tests the behaviours added in the example plugin.
+ */
+
+public class PluginExampleTest {


Java class names must match the file name. In the case of tests, we take the name of the class under test and add Test to the end.

kvosper · 2019-01-29T11:31:49Z

plugin-examples/src/test/java/com/hotels/styx/ExampleMessageInterceptTest.java

+public class PluginExampleTest {
+    @Test
+    public void returnsEarly() {
+        PluginExample plugin = new PluginExample();


The class PluginExample does not exist. Please ensure that code compiles and tests pass before creating a PR. In the case of these examples, it is important to provide code that we have confirmed functional, so as to avoid confusing anyone who tries to learn from it.

kvosper · 2019-02-07T10:46:37Z

plugin-examples/src/main/java/com/hotels/styx/EarlyReturnExamplePlugin

@@ -0,0 +1,50 @@
+/*


The file name should end in .java

The filename is still incorrect

kvosper · 2019-02-07T10:52:17Z

plugin-examples/src/main/java/com/hotels/styx/EarlyReturnExamplePlugin

+import static java.nio.charset.StandardCharsets.UTF_8;
+
+/**
+ * This shows an example of an interceptor that responds to a received request


I think we should contrast between the behaviour here and the alternative.

Suggested change

* This shows an example of an interceptor that responds to a received request

* This shows an example of an plugin that responds immediately to a received request, instead of proxying it downstream.

kvosper · 2019-02-07T10:54:02Z

plugin-examples/src/main/java/com/hotels/styx/EarlyReturnExamplePlugin

+
+/**
+ * This shows an example of an interceptor that responds to a received request
+ * It takes a request and if the HTTP request contains an x-respond in the header then it returns an eventual made of


Suggested change

* It takes a request and if the HTTP request contains an x-respond in the header then it returns an eventual made of

* It takes a request and if the HTTP request contains a header named `X-Respond`, then it returns an eventual wrapping

kvosper · 2019-02-07T10:54:21Z

plugin-examples/src/main/java/com/hotels/styx/EarlyReturnExamplePlugin

+/**
+ * This shows an example of an interceptor that responds to a received request
+ * It takes a request and if the HTTP request contains an x-respond in the header then it returns an eventual made of
+ * the response code, body etc.


Suggested change

* the response code, body etc.

* the HTTP response.

kvosper · 2019-02-07T10:56:44Z

plugin-examples/src/main/java/com/hotels/styx/EarlyReturnExamplePlugin

+ * This shows an example of an interceptor that responds to a received request
+ * It takes a request and if the HTTP request contains an x-respond in the header then it returns an eventual made of
+ * the response code, body etc.
+ * The eventual object is returned but the values only come into existance whe  the response return is received.


Suggested change

* The eventual object is returned but the values only come into existance whe the response return is received.

* The eventual object is returned immediately, even if the response has not yet arrived.

kvosper · 2019-02-07T11:14:55Z

plugin-examples/src/main/java/com/hotels/styx/ReplaceLiveContentExample.java

+    /**
+     * You can replace live content
+     * If you need to replace a message body based on information in the message headers and don't care what is in
+     * the message body. (E.g you need to add a HTTP message body based on a HTTP error code)


Suggested change

* the message body. (E.g you need to add a HTTP message body based on a HTTP error code)

* the original message body. (E.g you need to add a HTTP message body based on a HTTP error code)

kvosper · 2019-02-07T11:15:27Z

plugin-examples/src/main/java/com/hotels/styx/ReplaceLiveContentExample.java

+     * If you need to replace a message body based on information in the message headers and don't care what is in
+     * the message body. (E.g you need to add a HTTP message body based on a HTTP error code)
+     * <p>
+     * You can transform a live HTTP message body using a replaceWith Bytestream operator such as in the example.


Suggested change

* You can transform a live HTTP message body using a replaceWith Bytestream operator such as in the example.

* You can transform a live HTTP message body using the `replaceWith` Bytestream operator as shown in the example below.

kvosper · 2019-02-07T11:16:06Z

plugin-examples/src/main/java/com/hotels/styx/ReplaceLiveContentExample.java

+    }
+}
+
+/**


This comment should be at the top of the file, combined with the other comment.

kvosper · 2019-02-07T11:16:28Z

plugin-examples/src/main/java/com/hotels/styx/ReplaceLiveContentExample.java

+}
+
+/**
+This can be used to replace a message body without having to look into it, this will also save heap as the


Suggested change

This can be used to replace a message body without having to look into it, this will also save heap as the

* This can be used to replace a message body without having to look into it, which will also save heap space as the

kvosper · 2019-02-07T11:16:43Z

plugin-examples/src/main/java/com/hotels/styx/ReplaceLiveContentExample.java

+
+/**
+This can be used to replace a message body without having to look into it, this will also save heap as the
+ live upstream response body is never stored in heap in full


Suggested change

live upstream response body is never stored in heap in full

* live upstream response body is never stored in the heap in full.

dvlato · 2019-03-18T12:23:51Z

plugin-examples/src/main/java/com/hotels/styx/EarlyReturnExamplePlugin

@@ -0,0 +1,50 @@
+/*


The filename is still incorrect

dvlato · 2019-03-18T12:36:09Z

plugin-examples/src/main/java/com/hotels/styx/ReplaceContentByAggregationExample.java

+ *Please note that this uses more heap space as the full response is transiently stored in the heap.
+ */
+
+public class ReplaceContentByAggregationExample implements Plugin {


I cannot find any 'modifier' implementation so I don't think this is 'usable' code. Also the ModifyHeadersExamplePluginConfig field/attribute is not used.

dvlato · 2019-03-18T12:36:38Z

plugin-examples/src/main/java/com/hotels/styx/ReplaceLiveContentExample.java

+import static java.util.Objects.requireNonNull;
+
+/**
+ * You can replace without aggregating it first, if the replacement does not depend on the original contents.


You can replace... the body content?

kvosper · 2019-03-18T13:50:54Z

plugin-examples/src/main/java/com/hotels/styx/Modifier.java

+import com.hotels.styx.api.HttpResponse;
+
+/**
+ * Marker interface for classes that perform some modification


Suggested change

* Marker interface for classes that perform some modification

* An interface for classes that perform some modification

Marker interfaces are interfaces with no methods that just sort-of "mark" classes in some way (basically an old workaround before annotations were added to Java).

kvosper · 2019-03-18T14:08:25Z

plugin-examples/src/main/java/com/hotels/styx/ReplaceContentByAggregationExample.java

+    public Eventual<LiveHttpResponse> intercept(LiveHttpRequest request, Chain chain) {
+
+        chain.proceed(request)
+                .flatMap(response -> it.aggregate(10000))


This code has syntax errors and does not compile.

dvlato · 2019-05-30T10:32:15Z

plugin-examples/pom.xml

@@ -1,205 +1,82 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
+  <parent>


I believe we removed the 'parent' here so the pom file can be copied to real plugins that will not declare themselves as 'children' of styx-parent. Do we definitely need that?

dvlato · 2019-05-30T10:33:53Z

plugin-examples/src/main/java/com/hotels/styx/EarlyReturnExamplePlugin.java

+    @Override
+    public Eventual<LiveHttpResponse> intercept(LiveHttpRequest request, Chain chain) {
+        if (request.header("X-Respond").isPresent()) {
+            return Eventual.of(HttpResponse.response(OK)


Should we consume the request body here?

dvlato · 2019-05-30T10:36:13Z

plugin-examples/src/main/java/com/hotels/styx/ReplaceLiveContentExamplePlugin.java

+import static java.util.Objects.requireNonNull;
+
+/**
+ * You can replace without aggregating it first, if the replacement does not depend on the original contents.


I find the beginning a bit unclear. A suggestion you can quickly discard : (The) body content can be replaced without ... Something along those lines.

dvlato · 2019-05-30T10:38:27Z

plugin-examples/pom.xml

    </dependency>
    <dependency>
      <groupId>io.reactivex</groupId>
      <artifactId>rxjava</artifactId>
-      <version>${rxjava.version}</version>
-      <scope>provided</scope>
    </dependency>

    <dependency>
      <groupId>com.fasterxml.jackson.core</groupId>


As mentioned, all these changes in the dependencies mean that the pom file cannot be just copied to a new plugin. At some point we should make sure our BOM file does not expose all the dependencies of Styx and then have plugins import it...

kvosper · 2019-06-12T13:31:16Z

See #432

VivianLopes requested review from kvosper and dvlato January 29, 2019 09:29

kvosper suggested changes Jan 29, 2019

View reviewed changes

kvosper suggested changes Feb 7, 2019

View reviewed changes

dvlato suggested changes Mar 18, 2019

View reviewed changes

kvosper suggested changes Mar 18, 2019

View reviewed changes

VivianLopes added 3 commits May 22, 2019 14:23

added comments

675a669

made requested changes

4d6ead3

Made requested changes

fb087a5

VivianLopes force-pushed the pluginexample branch from a510b89 to fb087a5 Compare May 22, 2019 13:24

VivianLopes and others added 7 commits May 22, 2019 16:17

work in progress

56a2705

fixed ReplaceLiveContentExample

55518a5

fixed ModifyContentByAggregationExample

cac634b

Tidy-up

1aa3d82

modifyHeadersExamplePluginTest passes - todo:Why mvn build fails?

0fb3c4b

fix mvn build issue and checkstyle

7ad799f

Merge branch 'master' into pluginexample

84ecbc1

VivianLopes requested a review from mikkokar May 30, 2019 10:16

dvlato suggested changes May 30, 2019

View reviewed changes

dvlato reviewed May 30, 2019

View reviewed changes

kvosper closed this Jun 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added examples #374

added examples #374

VivianLopes commented Jan 29, 2019

kvosper Jan 29, 2019

kvosper Jan 29, 2019

kvosper Jan 29, 2019

kvosper Jan 29, 2019

kvosper Jan 29, 2019

kvosper Jan 29, 2019

kvosper Feb 7, 2019

dvlato Mar 18, 2019

kvosper Feb 7, 2019

kvosper Feb 7, 2019

kvosper Feb 7, 2019

kvosper Feb 7, 2019

kvosper Feb 7, 2019

kvosper Feb 7, 2019

kvosper Feb 7, 2019

kvosper Feb 7, 2019

kvosper Feb 7, 2019

dvlato Mar 18, 2019

dvlato Mar 18, 2019

dvlato Mar 18, 2019

kvosper Mar 18, 2019

kvosper Mar 18, 2019

dvlato May 30, 2019

dvlato May 30, 2019

dvlato May 30, 2019

dvlato May 30, 2019

kvosper commented Jun 12, 2019

	* This shows an example of an interceptor that responds to a received request
	* This shows an example of an plugin that responds immediately to a received request, instead of proxying it downstream.

	* It takes a request and if the HTTP request contains an x-respond in the header then it returns an eventual made of
	* It takes a request and if the HTTP request contains a header named `X-Respond`, then it returns an eventual wrapping

	* The eventual object is returned but the values only come into existance whe the response return is received.
	* The eventual object is returned immediately, even if the response has not yet arrived.

	* the message body. (E.g you need to add a HTTP message body based on a HTTP error code)
	* the original message body. (E.g you need to add a HTTP message body based on a HTTP error code)

	* You can transform a live HTTP message body using a replaceWith Bytestream operator such as in the example.
	* You can transform a live HTTP message body using the `replaceWith` Bytestream operator as shown in the example below.

+                  }
+              }
+              /**

	This can be used to replace a message body without having to look into it, this will also save heap as the
	* This can be used to replace a message body without having to look into it, which will also save heap space as the

	live upstream response body is never stored in heap in full
	* live upstream response body is never stored in the heap in full.

	* Marker interface for classes that perform some modification
	* An interface for classes that perform some modification


		//Example:

		chain.proceed(request)

added examples #374

added examples #374

Conversation

VivianLopes commented Jan 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvosper commented Jun 12, 2019