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

Fix devfile component preferences deserialization through GSON #14306

Merged
merged 8 commits into from
Sep 10, 2019

Conversation

mkuznyetsov
Copy link
Contributor

@mkuznyetsov mkuznyetsov commented Aug 22, 2019

What does this PR do?

Add TypeAdapterFactory for GSON in DtoFactory, that would support Serializable type, that we have in Devfile Preferences

Special Note regarding parsing of numbers:
It is known problem that by default all numeric types are parsed as double in Gson library, resulting in conversion of integer numbers like 1 into decimal point form 1.0 and rounding dome double type values. [1], [2]. This is a not a problem for JSON itself, since it is doesn't have strict typing, and as a numeric, those two values are identical. But to preserve user input maximally, we're added code that tries to parse value as an Long first, and than if wasn't succesfull, fallback to the Double form. This preserves the integer values, but may result that decimal zeroes may be trimmed, e.g. 1.00 will be converted into 1, which is for sure much more rare form of defining integer numbers.

[1] google/gson#1084
[2] https://stackoverflow.com/questions/24926786/make-gson-deserialize-numbers-to-integers-or-doubles

What issues does this PR fix or reference?

#14289

Release Notes

Docs PR

@che-bot che-bot added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/bug Outline of a bug - must adhere to the bug report template. labels Aug 22, 2019
@che-bot
Copy link
Contributor

che-bot commented Aug 22, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

  • build details
  • "che-server" docker image: maxura/che-server:14306

@che-bot
Copy link
Contributor

che-bot commented Aug 22, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:14306
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@tolusha
Copy link
Contributor

tolusha commented Aug 22, 2019

/cc @tolusha

@skabashnyuk
Copy link
Contributor

In general looks ok to me. @mshaposhnik @metlos @sleshchenko wdyt?

@mshaposhnik
Copy link
Contributor

Ok if it fixes the problem

@skabashnyuk skabashnyuk marked this pull request as ready for review August 29, 2019 08:10
@che-bot
Copy link
Contributor

che-bot commented Sep 2, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

  • build details
  • "che-server" docker image: maxura/che-server:14306

@mkuznyetsov
Copy link
Contributor Author

@sleshchenko @mshaposhnik @skabashnyuk @metlos
so there was a problem with this approach, using default ObjectAdapter all numbers would deserialize to long, even if they could be just integer. Unlike Jackson deserializer, which guesses the correct types in this case.

(there are numbers of issues in GSON repo for this, like this one google/gson#1084)

I'm not sure whether it's critical in our case, but I thought it might be important to have our parsers to behave the same way, so I added a custom type adapter with the last commit to fix this problem in the simplest option.

So the question is, do we need to bother with this at all, and if are there any better alternatives to it

@che-bot
Copy link
Contributor

che-bot commented Sep 2, 2019

E2E tests of Eclipse Che Multiuser on OCP has been successful:

@che-bot
Copy link
Contributor

che-bot commented Sep 2, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@che-bot
Copy link
Contributor

che-bot commented Sep 2, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

  • build details
  • "che-server" docker image: maxura/che-server:14306

@che-bot
Copy link
Contributor

che-bot commented Sep 2, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@che-bot
Copy link
Contributor

che-bot commented Sep 5, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@che-bot
Copy link
Contributor

che-bot commented Sep 5, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

  • build details
  • "che-server" docker image: maxura/che-server:14306

@che-bot
Copy link
Contributor

che-bot commented Sep 5, 2019

E2E tests of Eclipse Che Multiuser on OCP has been successful:

@che-bot
Copy link
Contributor

che-bot commented Sep 10, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@che-bot
Copy link
Contributor

che-bot commented Sep 10, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@metlos
Copy link
Contributor

metlos commented Sep 10, 2019

the latest master needs to be merged into this PR for the happy paths to succeed.

@che-bot
Copy link
Contributor

che-bot commented Sep 10, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

  • build details
  • "che-server" docker image: maxura/che-server:14306

@che-bot
Copy link
Contributor

che-bot commented Sep 10, 2019

E2E tests of Eclipse Che Multiuser on OCP has been successful:

@mshaposhnik
Copy link
Contributor

ci-build

@mshaposhnik mshaposhnik merged commit ec76c53 into master Sep 10, 2019
@che-bot che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Sep 10, 2019
@che-bot che-bot added this to the 7.2.0 milestone Sep 10, 2019
@mkuznyetsov mkuznyetsov deleted the che-14289 branch February 18, 2020 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Outline of a bug - must adhere to the bug report template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants