added POST option to forward url #3755

NateZ7 · 2018-02-04T13:32:01Z

Hi Anton,

As we discussed, I added the forward.json to determine POST or GET in forward url.

NateZ7

Refactored code

tananaev · 2018-02-04T20:27:48Z

Why are there two pull requests? Also, please address comment from the second thread.

NateZ7 · 2018-02-05T09:12:26Z

This one is the relevant PR. deleted the other one and fixed what you said, thanks.

tananaev · 2018-02-05T10:17:50Z

src/org/traccar/BasePipelineFactory.java

@@ -238,7 +238,9 @@ public ChannelPipeline getPipeline() {
        }

        if (Context.getConfig().getBoolean("forward.enable")) {
-            pipeline.addLast("webHandler", new WebDataHandler(Context.getConfig().getString("forward.url")));
+            final Boolean json = Context.getConfig().getBoolean("forward.json");


I think we can safely inline the variables.

Its longer than 150 lines, Travis CI didn't accept it

You can separate it into several lines.

That's what I did

tananaev · 2018-02-05T10:18:06Z

src/org/traccar/WebDataHandler.java

@@ -29,12 +32,17 @@
 import java.util.Locale;
 import java.util.TimeZone;

+


Remove extra spaces like this.

tananaev · 2018-02-05T10:18:27Z

src/org/traccar/WebDataHandler.java

 public class WebDataHandler extends BaseDataHandler {

    private final String url;
+    private final Boolean json;


Why is it a boolean class and not a primitive type?

tananaev · 2018-02-05T10:18:34Z

src/org/traccar/WebDataHandler.java


-    public WebDataHandler(String url) {
+    public WebDataHandler(String url, Boolean json) {


tananaev · 2018-02-05T10:19:07Z

test/org/traccar/WebDataHandlerTest.java

@@ -12,7 +12,7 @@ public void testFormatRequest() throws Exception {

        Position p = position("2016-01-01 01:02:03.000", true, 20, 30);

-        WebDataHandler handler = new WebDataHandler("http://localhost/?fixTime={fixTime}&gprmc={gprmc}&name={name}");
+        WebDataHandler handler = new WebDataHandler("http://localhost/?fixTime={fixTime}&gprmc={gprmc}&name={name}", false);


Please add unit test for JSON as well.

tananaev · 2018-02-05T10:19:52Z

src/org/traccar/WebDataHandler.java

        return position;
    }

+    protected String getContentType() {


Why do we need a separate method?

How would you want me to get the content type from 'JsonTypeEventForwarder' ??

What does JsonTypeEventForwarder have to do with this? It has nothing to do with events.

What did you mean by separate method ? maybe I dont get it.

Why do you have a method with one line of code? It doesn't make sense to me.

understood. Fixed and pushed

tananaev · 2018-02-05T10:19:58Z

src/org/traccar/WebDataHandler.java

+        return "application/json; charset=utf-8";
+    }
+
+    protected void setContent(Position position, AsyncHttpClient.BoundRequestBuilder requestBuilder) {


Same question.

Here, we need a separate method because I don't have Event object in WebDataHandler, so that method is edited accordingly

I don't understand what events have to do with this functionality.

What did you mean by separate method ? maybe I dont get it.

tananaev · 2018-02-06T19:34:50Z

You haven't fixed one of the comments (about removing variables). Also, why is test case called testBuilderRequest?

NateZ7 · 2018-02-07T07:15:35Z

Thanks for the comment, Fixed the test's name to match previous syntax, plus fixed the inlined variables.

tananaev · 2018-02-07T07:24:52Z

@Abyss777, can you please take a quick look and see if you can think of any issues or problems.

Abyss777 · 2018-02-07T07:31:05Z

What is the reason of sending Device object with every position?

NateZ7 · 2018-02-07T07:34:52Z

@Abyss777 We got a request from a client that matches the data we get from the position, and maybe it will be helpful for others. Nevertheless, there is a flag to activate it or not.

tananaev · 2018-02-07T07:38:39Z

I think it makes sense so that third party app has access to device model as well, if it needs. Also, I imagine most of the time this will be used on a same machine or at least local network, so some extra traffic shouldn't be a problem.

Also, it's consistent with event forwarding.

Abyss777 · 2018-02-07T07:42:22Z

src/org/traccar/WebDataHandler.java

+            data.put(KEY_POSITION, position);
+        }
+
+        if (position.getDeviceId() != 0) {


I think this check if not necessary and next check device for null be enough .

@Abyss777, should we remove same checks from EventForwarder?

@Abyss777 Again, the check of the position is not necessary ? just leave the check device for null ?

I think it's safe to assume that there is a device.

So remove both checks ?

Hm...I believe yes we can remove.
Map.get(0) will return null - "the value to which the specified key is mapped, or null if this map contains no mapping for the key"

Abyss777 · 2018-02-07T07:42:44Z

OK. Then one small comment.

NateZ7 · 2018-02-07T08:27:48Z

@Abyss777 remove the checks.

tananaev · 2018-02-07T08:30:37Z

What about device? Why are we removing it?

Abyss777 · 2018-02-07T08:56:09Z

@NateZ7
You need remove checks id for 0, but leave checks objects for null

NateZ7 · 2018-02-07T09:14:12Z

@Abyss777 Got ya ! did the changes

tananaev · 2018-02-07T09:19:47Z

Merged it, thanks.

slobglob · 2018-04-07T17:10:44Z

@tananaev Please add the forward.json option to https://www.traccar.org/configuration-file/
Thank you.

tananaev · 2018-04-07T21:28:18Z

Updated.

NateZ7 added 4 commits February 4, 2018 15:31

added POST option to forward url

377a223

Fixed length on basepipelinefactory

dc09213

Code refactoring

fc3106e

code refactoring

6ae8a39

NateZ7 commented Feb 4, 2018

View reviewed changes

changed POST to use full object

00b1715

tananaev reviewed Feb 5, 2018

View reviewed changes

NateZ7 added 2 commits February 5, 2018 13:35

added unit test, fixed spaces, fixed primitive types

ed4a723

removed unneccesary methods

4969ecd

fixed inline variables + test name

da1f01f

Abyss777 reviewed Feb 7, 2018

View reviewed changes

NateZ7 added 2 commits February 7, 2018 09:56

remove unnecasseray checks

3a0a269

fixed test + removed checks

f41ed8f

Fixed correct checks

56669ce

tananaev merged commit 96e45b5 into traccar:master Feb 7, 2018

		@@ -29,12 +32,17 @@
		import java.util.Locale;
		import java.util.TimeZone;


		public WebDataHandler(String url) {
		public WebDataHandler(String url, Boolean json) {

added POST option to forward url #3755

added POST option to forward url #3755

Conversation

NateZ7 commented Feb 4, 2018

NateZ7 left a comment

Choose a reason for hiding this comment

tananaev commented Feb 4, 2018

NateZ7 commented Feb 5, 2018

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

tananaev commented Feb 6, 2018

NateZ7 commented Feb 7, 2018 • edited

tananaev commented Feb 7, 2018

Abyss777 commented Feb 7, 2018

NateZ7 commented Feb 7, 2018

tananaev commented Feb 7, 2018

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

Abyss777 commented Feb 7, 2018

NateZ7 commented Feb 7, 2018

tananaev commented Feb 7, 2018

Abyss777 commented Feb 7, 2018

NateZ7 commented Feb 7, 2018

tananaev commented Feb 7, 2018

slobglob commented Apr 7, 2018 • edited

tananaev commented Apr 7, 2018

NateZ7 commented Feb 7, 2018 •

edited

slobglob commented Apr 7, 2018 •

edited