Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Update Kotlin SDK to use Gson instead of Jackson #836

Merged
merged 9 commits into from
Sep 26, 2021

Conversation

mistryrakesh
Copy link
Contributor

@mistryrakesh mistryrakesh commented Sep 24, 2021

This PR updates the kotlin SDK to use Gson instead of Jackson for deserialization (We were already using gson to serialize).

This fixes:

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Appropriate docs were updated (if necessary)

Fixes #825 🦕

@google-cla google-cla bot added the cla: yes label Sep 24, 2021
@github-actions
Copy link
Contributor

Codegen Tests

  1 files    6 suites   59s ⏱️
58 tests 46 ✔️ 12 💤 0 ❌

Results for commit e31ccff.

Copy link
Contributor

@jkaster jkaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great update. Nice to have cleaner and more flexible code.

bin/smoke kotlin

passes.

Can you add a test that verifies the graceful handling of unknown fields?

@github-actions
Copy link
Contributor

Codegen Tests

  1 files  ±0    6 suites  ±0   59s ⏱️ +9s
58 tests ±0  46 ✔️ ±0  12 💤 ±0  0 ❌ ±0 

Results for commit 870ab75. ± Comparison against base commit 283cbd5.

@mistryrakesh
Copy link
Contributor Author

Hey @jkaster thanks for reviewing!

I have added a smoke test (SmokeTest.testIgnoreUnknownFields()) which basically fetches all users and deserializes it into DummyUser object. The DummyUser class is a subset of the User class. When the server sends a response User object the Gson deserializer ignores extra properties and creates the DummyUser object.

Using the Jackson deserializer the test fails with error:

java.lang.Error: GET /users com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized field "avatar_url" (class DummyUser), not marked as ignorable (4 known properties: "display_name", "id", "credentials_api3", "email"])
 at [Source: (StringReader); line: 1, column: 1260] (through reference chain: java.lang.Object[][0]->DummyUser["avatar_url"]).message
java.lang.AssertionError: java.lang.Error: GET /users com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized field "avatar_url" (class DummyUser), not marked as ignorable (4 known properties: "display_name", "id", "credentials_api3", "email"])
 at [Source: (StringReader); line: 1, column: 1260] (through reference chain: java.lang.Object[][0]->DummyUser["avatar_url"]).message
	at org.junit.Assert.fail(Assert.java:88)
	at kotlin.test.junit.JUnitAsserter.fail(JUnitSupport.kt:56)
	at kotlin.test.Asserter$DefaultImpls.assertTrue(Assertions.kt:226)
	at kotlin.test.junit.JUnitAsserter.assertTrue(JUnitSupport.kt:30)
	at kotlin.test.Asserter$DefaultImpls.assertTrue(Assertions.kt:236)
	at kotlin.test.junit.JUnitAsserter.assertTrue(JUnitSupport.kt:30)
	at kotlin.test.AssertionsKt__AssertionsKt.assertTrue(Assertions.kt:40)
	at kotlin.test.AssertionsKt.assertTrue(Unknown Source)
	at TestSmoke.testIgnoreUnknownFields(TestSmoke.kt:212)

@jkaster
Copy link
Contributor

jkaster commented Sep 25, 2021

@mistryrakesh This is great stuff! I really appreciate your contributions here.

Copy link
Contributor

@jkaster jkaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done! It's great to have better resilience for the Kotlin SDK.

kotlin/src/test/TestSmoke.kt Show resolved Hide resolved
@mistryrakesh mistryrakesh merged commit 4d1f789 into main Sep 26, 2021
@mistryrakesh mistryrakesh deleted the rakeshmistry/kt_use_gson branch September 26, 2021 02:11
@github-actions

This comment has been minimized.

jkaster added a commit that referenced this pull request Sep 30, 2021
* feat: request body validation before Run

also fixed RunIt form update when methods are changed

* fixed e2e tests but lost type tag tests

Need to expand e2e testing anyway

* moving prettier to scripts package

sadly, no version of it can run in the browser

* chore: refreshing hackathon app (#835)

* chore: refreshing hackathon app

also

 - updated TypeScript
 - updated Prettier
 - bumped ts-jest
 - updated ts-node

* Fixing APIX CI workflow

* feat: Update Kotlin SDK to use Gson instead of Jackson (#836)

* Updated Kotlin SDK to use Gson for serialization/deserialization.
* Added type adapter for AuthToken to handle constructor calls.
* Added smoke test to check deserialization ignores unknown properties.

* fix type resolution in various test:sdk suites

* fix: Property names with special characters in Kotlin SDK (#838)

This PR fixes generation of kotlin classes which handle properties having special characters.

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
- [x] Make sure to open an issue as a [bug/issue](https://github.com/looker-open-source/sdk-codegen/issues/new/choose) before writing your code!  That way we can discuss the change, evaluate designs, and agree on the general idea
- [x] Ensure the tests and linter pass
- [x] Appropriate docs were updated (if necessary)

Fixes #837 🦕

* added form encoded body validation

* allow . and - in encoded argument names

Co-authored-by: Rakesh Mistry <rakesh.dta@gmail.com>
Co-authored-by: Rakesh Mistry <rakeshmistry@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android App should Gracefully Handle Extra SDK Parameters
2 participants