schnatterer / musicbrainzws2-java

java binding for MusicBrainz XML Web Service/Version 2
GNU General Public License v3.0
15 stars 10 forks source link

Fix encoding of post body. #2

Closed RillingDev closed 3 years ago

RillingDev commented 3 years ago

Hi! Thanks for the great library. I was having issues submitting tags the other day, the tag "kayōkyoku" was always sent as "kay?kyoku". I did some investigation and found that while the content type header for this request is set to UTF-8, the body uses the default encoding in the StringEntity constructor, which is ISO-8859-1. This patch changes the encoding of the body to be UTF-8.

RillingDev commented 3 years ago

Thank you for the very fast response!

I had a quick glance at the code but apart from that I can't judge if this works. There are no unit tests and I honestly can't event try if this works. I only ever sent GET requests to MB see_no_evil

Can you tell me how I can find out if this works?

Sure! Here is a simple snippet which fails in the current version that works after the patch:

        ReleaseGroup releaseGroup = new ReleaseGroup();
        ReleaseGroupIncludesWs2 includes = new ReleaseGroupIncludesWs2();
        includes.includeAll();
        releaseGroup.setIncludes(includes);
        releaseGroup.getQueryWs().setClient("TestyMcTestface/0.0.1");
        releaseGroup.getQueryWs().setUsername("username");
        releaseGroup.getQueryWs().setPassword("password");

        releaseGroup.lookUp("mbid");

        releaseGroup.AddTags(new String[]{"kayōkyoku"});
schnatterer commented 3 years ago

I implemented a way of running the existing integration test (it's called UnitTest :see_no_evil: ) against test.musicbrainz.org. Could you refactor the snippet you wrote in your comment into a test? That way we can prohibit regressions. To do so, please rebase the upstream branch on yours.

Downside is that you'll probably need an account at test.musicbrainz.org for development. In future this might be improved by using a containerized local MusicBrainz instance. I just don't have the time right now.

After you done, please squash to one commit. Then I'll release a new version. It's going to be available on Maven Central BTW :)

RillingDev commented 3 years ago

Added test and rebased into one commit. 1) I added a test without assertions (just using stdouts like the other tests), is that ok? 2) Travis build failed, but I think that is not due to my new test

schnatterer commented 3 years ago

Thanks for the test!

  1. I added an assertion. Otherwise the test would not fail with kay?kyoku, which was the reason for the whole thing.
  2. Yes, travis failure was due to the credentials for the test server being passed in by travis via env vars, which is disabled for PRs. Luckily, I found out that all passwords on the test server are the same :grimacing: (well, good for us)
schnatterer commented 3 years ago

Thanks again. New version now released to maven central (you can get rid of the repo in pom.xml)

<dependency>
    <groupId>info.schnatterer.musicbrainzws2-java</groupId>
    <artifactId>musicbrainzws2-java</artifactId>
    <version>3.0.2</version>
</dependency>
RillingDev commented 3 years ago

Thank you!