SparkPost / java-sparkpost

SparkPost client library for Java
https://www.sparkpost.com/
Other
39 stars 35 forks source link

Resource could not be found error #57

Closed olegkon closed 7 years ago

olegkon commented 7 years ago

Hi,

Trying to run the sample with sparky.sendMessage() and getting the following error:

{ "errors": [ { "message": "Resource could not be found" } ] }

Looks like the code is adding an extra slash (before transmissions) into the url calling https://api.sparkpost.com/api/v1//transmissions?num_rcpt_errors=0

We are using apache httpclient-4.5.3.jar (on Win 10).

Can anyone please fix it?

TIA, Oleg.

yepher commented 7 years ago

@olegkon I just ran this sample after setting my API key, proper from address and a proper too address and it worked fine: sendMessage()

Can you provide a little more details for how you are using the library. Version and if you are using Maven will be helpful too.

To make sure I set a break point here RestConnection.java#L112 and I do not see the extra / that you are seeing.

screen shot 2017-03-02 at 8 09 10 pm

Is it possible you set a trailing slash for SPARKPOST_BASE_URL=https://api.sparkpost.com/api/v1 in config.properties or have that set as an environment variable with the trailing slash?

yepher commented 7 years ago

@olegkon, I've spent time looking through the library code for how we might get an extra / and I am not finding any suspicious code yet. In fact the code guards against that case.

Early in the libraries history there was a case where there was a duplicate / but that was fixed long ago. I did a quick search but did not find the exact commit where that was fixed. But here is the code that is meant to address it: RestConnection.java#L90

public RestConnection(Client client, String endpoint) throws SparkPostException {
        this.client = client;
        if (StringUtils.isAnyEmpty(endpoint)) {
            this.endpoint = defaultApiEndpoint;
        } else {
            if (endpoint.endsWith("/")) {   // Here we protect agains a trailing slash if not from SPARKPOST_BASE_URL property
                this.endpoint = endpoint;
            } else {
                this.endpoint = endpoint + '/';
            }
        }
    }

My best guess is there is a / on the end of this variable SPARKPOST_BASE_URL that your environment has set by one or more of the following:

* JVM argument
* Env variable
* `config.properties` file.

OR

You are running a very old version of the library.

I am going to leave this issue open until next week to give you some time to provide additional feedback. I would be glad to help you get it working and or fix the code if there is indeed a bug in the library. If you are on "http://slack.sparkpost.com/" mention me there (@chris.wilson-sp) and I will jump on and try and help you real-time.

Starting countdown to close issue as not an issue if no addition details are provided...

dangerusslee commented 7 years ago

@yepher If that code is supposed to do what it says it in the comment, then it's flipped. That code is making sure that there is a slash at the end of the endpoint.

So when there's a leading slash in the path as well, it combines to form //

I believe createConnectionObject should have a guard to make sure there's no // or you need to make sure that the path doesn't have a leading slash.

I believe a difference in the version of httpclient may be causing you to not see the issue.

yepher commented 7 years ago

Sorry. I mis-typed the comment when responding to the issue. I meant to say it only adds the / if it not already there.

I will test with that httpclient version and report back shortly.

olegkon commented 7 years ago

Guys, it adds another part to it (path?) with transmissions, which has another '/' (/transmissions?num_rcpt_errors=0) Sorry, don't have debugger now, can't recall exactly, saw it yesterday.

I don't have any config file. Should I use it? Please give more details, I am totally new to SparkPost.

dangerusslee commented 7 years ago

@yepher It's also possible that you have a config file and it gets the defaultApiEndpoint from there which doesn't have a trailing slash.

yepher commented 7 years ago

hmm, EndPoint unit tests fail with httpclient-4.5.3.

Will do more investigation:

Running com.sparkpost.EndPointTest
Tests run: 4, Failures: 4, Errors: 0, Skipped: 0, Time elapsed: 5.51 sec <<< FAILURE!
testEndPointWithMultipleParameters(com.sparkpost.EndPointTest)  Time elapsed: 0.028 sec  <<< FAILURE!
org.junit.ComparisonFailure: expected:<[]transmissions?num_rc...> but was:<[/]transmissions?num_rc...>
    at com.sparkpost.EndPointTest.testEndPointWithMultipleParameters(EndPointTest.java:84)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:252)
    at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:141)
    at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:112)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.apache.maven.surefire.util.ReflectionUtils.invokeMethodWithArray(ReflectionUtils.java:189)
    at org.apache.maven.surefire.booter.ProviderFactory$ProviderProxy.invoke(ProviderFactory.java:165)
    at org.apache.maven.surefire.booter.ProviderFactory.invokeProvider(ProviderFactory.java:85)
    at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:115)
    at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:75)
yepher commented 7 years ago

I am working a new release. I will fix this and get it into the next release. Thank you @dangerusslee and @olegkon.

I am going to make sure it works with httpclient-4.5.3 and httpclient-4.5 but I will also bump the pom dependency up the latest version of HTTP Client.

yepher commented 7 years ago

Back testing HTTP CLIENT 4.5.x

yepher commented 7 years ago

This is the HTTP Client change that break backward behavior: https://github.com/apache/httpclient/commit/0554271750599756d4946c0d7ba43d04b1a7b220

yepher commented 7 years ago

I was getting ready to send an email to olegk at apache.org to verify the new HTTP client URIBuilder is performing the expected behavior when I noticed the similarity in names:


ChangeLog:


@olegkon are you the same Apache olegk that made URIBuilder change or is that coincidence?

olegkon commented 7 years ago

No, I am olegkon@yahoo.com Oleg is a popular name ;-)

yepher commented 7 years ago

From Oleg at Apache:

This change was made per this bug report [1]. You have already found
out that. There was a follow up change request post 4.5.3 that adjusted
URIBuilder behavior slightly [2]. 

Going forward (4.5.4) URIBuilder will make sure that a path if non-null
will always starts with /. Null path will not be altered or converted
to /.

Hope this helps

Oleg

[1] https://issues.apache.org/jira/browse/HTTPCLIENT-1803
[2] https://issues.apache.org/jira/browse/HTTPCLIENT-1810

So I will add code to handle <4.5.3 and >=4.5.3 HTTP Client. Eventually will remove support for HTTPClient 4.5.3.

olegkon commented 7 years ago

When do you plan to release it, so we can use that JAR?

yepher commented 7 years ago

@olegkon I am working on unit tests for each of the resource classes so I can do regression testing easier. My plan is to release a new version mid next week. I hope to be done coding sometime tomorrow. But I need some time to run the integration tests for both old version of HTTP client and new version.

I have five more classes to go. Three of them are pretty easy to test but one has a lot of methods to test.

This commit 8bf9501 works fine but I did not like it (even though it was an easer fix). I am instead going to standardize how the URI's are built and subject them all to unit tests.

yepher commented 7 years ago

@olegkon Incase you get a chance to test here is a pre-release jar with the changes. I would be interested to hear any feedback you may have.

sparkpost-lib-0.17-pre.zip

olegkon commented 7 years ago

@yepher I tested your fix with httpclient 4.5.3, and it worked, all except that when I used both text and html parameter of sparky.sendMessage(from, to, subject, text, html) , I did not get any text inside the email body, only html part. Is it how it is supposed to work? if so, what it text parameter for?

Here is email source:

some HTML <--- this is correct HTML I sent.

TIA, Oleg.

yepher commented 7 years ago

You should get the text part and the HTML part. I just tried it and it worked for me.

Can you post the JSON that is sent?

yepher commented 7 years ago

@olegkon If you are still having problems with plain text not getting sent with HTML can you please create a separate issue. Also if you can provide the JSON that gets sent when that happens that would help a lot.

Incase you did not know you can get the JSON by setting this logger com.sparkpost.transport.RestConnection to DEBUG.

yepher commented 7 years ago

Fixed in Version 0.17.

amitthakur4820 commented 6 years ago

I am getting this error while creating sparkpost interface on jsp and servlet org.apache.tomcat.websocket.server.WsFilter.doFilter(WsFilter.java:52)