Upgrade Jackson 1.x to Jackson 2.x #22

gilday · 2017-01-30T22:30:33Z

#21 Upgrade Jackson

I was unable to run the integration tests due to problems with the API rate limiter. Is this a known issue?

gilday · 2017-02-01T00:49:36Z

Travis appears to be failing because Travis has been set-up for this repository but the travis branch has yet to be merged

chrismetcalf · 2017-02-02T06:14:32Z

@raganw @chitang @znep Would one of you guys be willing and able to give this a code review?

bowenli86 · 2017-02-13T06:37:24Z

This PR is awesome! We should get this out asap!

I'm hving trouble because Jackson 1.x is too old and doesn't get things done well. Need Jackson 2.x!

bowenli86 · 2017-02-13T06:41:14Z

src/main/java/com/socrata/api/HttpLowLevel.java

@@ -78,6 +81,8 @@

    public static final GenericType<List<Object>> MAP_OBJECT_TYPE = new GenericType<List<Object>>() {};

+    private final ObjectMapper mapper;


Minor suggestion: since ObjectMapper is stateless, you can maintain a single ObjectMapper object as singleton across the code base.

Not critical. Can push it off to next release

Agreed, but since this isn't a new field, I was considering this to be out of scope; however, I did add a new constructor signature (line 188) so that the application may provide the library with its singleton ObjectMapper

chitang · 2017-02-13T22:59:05Z

src/main/java/com/socrata/api/Soda2Producer.java

@@ -76,6 +78,7 @@ public static final Soda2Producer newProducerWithRequestId(final String url, Str
    public Soda2Producer(HttpLowLevel httpLowLevel)
    {
        super(httpLowLevel);
+        factory = new JsonFactory();


This class needs the MappingJsonFactory. You will need to either get that from ObjectMapper or explicitly create one. You can see test failures in Soda2ProducerTest if you just use the plain JsonFactory.

Again, I need some help running the tests. When I try to run them, I get a whole lot of these failures in my terminal

[info] Test com.socrata.DatesTest.testFloatingDateSoql started [error] Test com.socrata.DatesTest.testFloatingDateSoql failed: Authentication failed: Invalid username or password [error] at com.socrata.api.HttpLowLevel.processErrors(HttpLowLevel.java:687) [error] at com.socrata.api.HttpLowLevel.queryRaw(HttpLowLevel.java:448) [error] at com.socrata.api.Soda2Base.query(Soda2Base.java:62) [error] at com.socrata.api.Soda2Consumer.query(Soda2Consumer.java:111) [error] at com.socrata.DatesTest.testFloatingDateSoql(DatesTest.java:52)

I didn't see anything in the README about how to avoid this. There are credentials in TestConfig.properties, but they don't appear to be working

For Soda2ProducerTest, you can supply your own username, password and token in TestConfig.properties.

OK, I guess the standard procedure is to edit this file with my credentials, but do not check those changes in

I tried this and while more tests do pass, some now report authorization problems

[info] Test com.socrata.api.SodaImporterTest.testCreatingFileDataset started [error] Test com.socrata.api.SodaImporterTest.testCreatingFileDataset failed: You aren't allowed to create a dataset within this domain. [error] at com.socrata.api.HttpLowLevel.processErrors(HttpLowLevel.java:689) [error] at com.socrata.api.HttpLowLevel.postFileRaw(HttpLowLevel.java:532) [error] at com.socrata.api.SodaImporter$4.issueRequest(SodaImporter.java:570) [error] at com.socrata.api.SodaImporter.importNonDataFile(SodaImporter.java:590) [error] at com.socrata.api.SodaImporterTest.testCreatingFileDataset(SodaImporterTest.java:425)

Also, what version of Java does this API target?

@gilday Did getting set up on sandbox unblock you? Anything more I can do to help?

i haven't had a chance to test it yet. i'll let you know this week

still having trouble with these tests. For example, the ExamplesTest#readmeProducerExamples uses a hardcoded username and password and the test fails with "Too many login attempts for that login. Account temporarily disabled."

Could you push the change? I can test the change.

pushed 5d70349

This is a bit of a struggle because some of the tests are also broken on master so I'm not sure what tests are broken because of my changes are which are just broken. For example, DatesTest#testDateCrud

chitang · 2017-02-24T21:20:52Z

src/main/java/com/socrata/api/SodaDdl.java

@@ -355,16 +352,9 @@ public ClientResponse issueRequest() throws LongRunningQueryException, SodaError

        try {
            final ClientResponse response = requester.issueRequest();
-            return mapper.readValue(response.getEntity(InputStream.class), AssetResponse.class);
-            //return response.getEntity(AssetResponse.class);
+            return response.getEntity(AssetResponse.class);


response.getEntity doesn't work with creating assets /api/assets. Keep the original mapper.readValue and error handlers.

I cannot find any documentation for the "assets" resource of the Import API. Does response.getEntity not work because this resource does not accept requests for application/json so the MessageBodyReader will not be triggered?

jackson looks for json content-type in getEntity. But the actual content type returned from the server is text/plain despite that it is json. Using readValue from input stream get around this problem.

chitang · 2017-02-24T21:38:44Z

src/test/java/com/socrata/api/Soda2ProducerTest.java

@@ -208,14 +210,15 @@ public void testDelete() throws LongRunningQueryException, SodaError, Interrupte
    @Test
    public void testParsingUpsertResults() throws IOException
    {
+        final Soda2Producer producer = new Soda2Producer(null);


Cannot be null. Need HttpLowLevel.

chitang · 2017-02-24T21:38:57Z

src/test/java/com/socrata/api/Soda2ProducerTest.java

@@ -245,22 +248,23 @@ public void testParsingUpsertResults() throws IOException
    @Test
    public void testParsingSodaServerUpsertResults() throws IOException
    {
+        final Soda2Producer producer = new Soda2Producer(null);


Cannot be null. Need HttpLowLevel.

chitang · 2017-02-24T21:39:20Z

src/test/java/com/socrata/api/Soda2ProducerTest.java

@@ -279,22 +283,23 @@ public void testParsingSodaServerUpsertResults() throws IOException
    @Test
    public void testParsingOldSodaServerUpsertResults() throws IOException
    {
+        final Soda2Producer producer = new Soda2Producer(null);


Cannot be null. Need HttpLowLevel.

chitang · 2017-02-24T21:43:31Z

src/test/java/com/socrata/utils/TestJacksonObjectMapperProvider.java


    @BeforeClass
    public static void setupClass() {
        // Make sure the ObjectMapper is setup correctly
-        final JacksonObjectMapperProvider provider = new JacksonObjectMapperProvider();
+        final JacksonObjectMapperProvider provider = new JacksonObjectMapperProvider(mapper);


This mapper can be null and is null when I run TestJacksonObjectMapperProvider.

* adding missing HttpLowLevel to Soda2ProducerTest * initialize ObjectMapper for TestJacksonObjectMapperProvider * provide a Jersey client filter for replacing response headers 'Content-Type: text/plain' with 'Content-Type: application/json' for the "assets" API so that it will un-marshall the AssetResponse type correctly

gilday · 2017-02-25T22:58:50Z

Tests that fail on master

Test com.socrata.DatesTest.testDateCRUD failed: null
Test com.socrata.api.SodaImporterTest.testSettingPrimaryKey failed: null expected:<Fun> but was:<null>
Test com.socrata.api.SodaDdlTest.testSearch failed: expected:<4> but was:<3>
Test com.socrata.ExamplesTest

com.socrata.ExamplesTest fails because it uses hard-coded credentials. I changed it to extend TestBase and use the user provided credentials and it passes

As for the other 3 tests, I do not know why they fail on both master and the gilday/21-upgrade-jackson branch

chitang · 2017-02-26T01:18:09Z

src/test/java/com/socrata/api/SodaImporterTest.java

@@ -18,7 +18,6 @@
 import junit.framework.TestCase;


Change from "new File" to "Resources.file" in line #37-43

chitang · 2017-02-26T01:45:55Z

src/test/java/com/socrata/utils/TestJacksonObjectMapperProvider.java


-    @Test
-    public void testMapperProvider() {
+    private static final Date base = new Date(112, 5, 20, 7, 0);


new Date is deprecated. Replace with Calendar.set

So, I moved this instantiation from elsewhere in the file as opposed to introducing it; therefore, can we consider this change out of scope?

There are a lot more instances of new Date(...) in the source and I expect you would want to address these at the same time as part of a comprehensive upgrade of the Date / Time system used by the library. For instance, you may consider replacing Joda Time with the new Java 8 java.time package. Java 7 is very old at this point, but if that's still important, you could consider the 310 back port

Fair enough

chitang · 2017-03-02T01:04:32Z

src/main/java/com/socrata/api/AssetRequestFilter.java

+        }
+        return response;
+    }
+}


Can we switch back to object mapper readValue from inputstream in SodaDdl.addAsset instead of a request filter?

chitang · 2017-03-10T01:09:03Z

src/main/java/com/socrata/api/AssetRequestFilter.java

+        }
+        return response;
+    }
+}


I prefer going back to object mapper readValue to adding a new AssetRequestFilter class. We can merge the PR once you make that change.

unmarshal the response from add asset instead of using a JAX-RS filter

gilday · 2017-03-10T02:25:00Z

Done

Upgrade Jackson 1.x to Jackson 2.x

f0ba424

bowenli86 approved these changes Feb 13, 2017

View reviewed changes

chitang reviewed Feb 13, 2017

View reviewed changes

chrismetcalf assigned chrismetcalf and chitang and unassigned chrismetcalf Feb 14, 2017

WIP working on getting the tests to pass

5d70349

chitang reviewed Feb 24, 2017

View reviewed changes

chitang reviewed Feb 26, 2017

View reviewed changes

chitang reviewed Mar 2, 2017

View reviewed changes

chitang reviewed Mar 10, 2017

View reviewed changes

maintainer feedback: use an instance of ObjectMapper to explicitly

8b123c5

unmarshal the response from add asset instead of using a JAX-RS filter

chitang merged commit 2751c01 into socrata:master Mar 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade Jackson 1.x to Jackson 2.x #22

Upgrade Jackson 1.x to Jackson 2.x #22

gilday commented Jan 30, 2017

gilday commented Feb 1, 2017

chrismetcalf commented Feb 2, 2017

bowenli86 commented Feb 13, 2017

bowenli86 Feb 13, 2017

bowenli86 Feb 13, 2017

gilday Feb 14, 2017

chitang Feb 13, 2017

gilday Feb 14, 2017

chitang Feb 14, 2017

gilday Feb 14, 2017

gilday Feb 14, 2017

chrismetcalf Feb 18, 2017

gilday Feb 20, 2017

gilday Feb 23, 2017

chitang Feb 23, 2017

gilday Feb 24, 2017

chitang Feb 24, 2017

gilday Feb 25, 2017

chitang Feb 26, 2017

chitang Feb 24, 2017

chitang Feb 24, 2017

chitang Feb 24, 2017

chitang Feb 24, 2017

gilday commented Feb 25, 2017

chitang Feb 26, 2017

chitang Feb 26, 2017

gilday Mar 1, 2017

chitang Mar 1, 2017

chitang Mar 2, 2017

chitang Mar 10, 2017

gilday commented Mar 10, 2017

		@@ -78,6 +81,8 @@

		public static final GenericType<List<Object>> MAP_OBJECT_TYPE = new GenericType<List<Object>>() {};

		private final ObjectMapper mapper;

Upgrade Jackson 1.x to Jackson 2.x #22

Upgrade Jackson 1.x to Jackson 2.x #22

Conversation

gilday commented Jan 30, 2017

gilday commented Feb 1, 2017

chrismetcalf commented Feb 2, 2017

bowenli86 commented Feb 13, 2017

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

gilday commented Feb 25, 2017

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

gilday commented Mar 10, 2017