Do not serialize DTO fields with empty or null objects and arrays #13515

mkuznyetsov · 2019-06-11T12:56:08Z

Signed-off-by: Mykhailo Kuznietsov mkuznets@redhat.com

What does this PR do?

currently serialization of devfileDto creates problems, when null or empty fields with collection/map types get serialized, which results in invalid Devfile.
In PR, special adapters for GSON would exclude fields with null or empty collections/maps from serialization.

Also added additional Enviroment Variable that can be used to revert the old behaviour:
CHE_LEGACY__DTO__JSON__SERIALIZATION (if true, empty and null collections and maps will be serialized)

What issues does this PR fix or reference?

Signed-off-by: Mykhailo Kuznietsov <mkuznets@redhat.com>

todo · 2019-06-11T12:56:11Z

fix

https://github.com/eclipse/che/blob/45f1c4971a02a8f2fb4fffe04e777478e839525d/core/che-core-api-dto/src/test/java/org/eclipse/che/dto/ServerDtoTest.java#L175-L178

This comment was generated by todo based on a `TODO` comment in `45f1c49` in #13515. cc @eclipse.

core/che-core-api-dto/src/main/java/org/eclipse/che/dto/server/NullOrEmptyMapAdapter.java

sleshchenko

In general, I'm OK with these changes. Let's run ci-test to check if it does not break current functionalities

Signed-off-by: Mykhailo Kuznietsov <mkuznets@redhat.com>

mkuznyetsov · 2019-06-11T13:49:57Z

ci-test

che-bot · 2019-06-11T17:10:26Z

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

metlos · 2019-06-12T10:50:13Z

.../che-core-api-dto/src/main/java/org/eclipse/che/dto/server/NullOrEmptyCollectionAdapter.java

+  @Override
+  public JsonElement serialize(List<?> src, Type typeOfSrc, JsonSerializationContext context) {
+    if (src == null || src.isEmpty()) {
+      return null;


I'm not sure this is correct. Isn't there a semantic difference between null and empty list? While it may solve an issue for stuff within the devfile, I'm not sure it won't cause trouble for other types of entities...

According to our DTOs impl - there is not any semantic difference between null or empty list/map. I mean null and empty collection/map can be used only as initialized collection/map (not null) after parsing.
From Che Server and GWT (client impls) perspective, nothing is changed.
For CLI that we used to use nothing is changed as well since typescript DTO generator initializes null fields.

The things that may be affected:

UD: it uses received JSON directly via interface without using DTO impls, and null collections/maps are not initialized. So, code like workspace.devfile.properties["persistVolumes"] = false may cause a failure, but I'm not sure. Thank @ashumilova, who agreed to check this and provide more info how it may affect UD;

Any other Che Server API client that relies on current DTO serialization style where all fields are set, even if they were not set initially;

I think we can stop serialization empty collections/maps if does not cause any ingressions.
What are the alternatives?:

stop using the same DTO object for all kind of components types and have like different classes for CheEditorComponentDto, ChePluginComponentDto, .... I do not have a clear picture of how it could be implemented;

make ComponentDto - a special DTO that has untypical behavior for DTOs (null, empty collections/maps are not serialized). It looks safer against the implemented solution, but it makes our DTOs behavior variational from class to class, which is not a good thing IMHO.

Do not use DTOs for devfile at all, and it's not clear what we can use instead, especially if we take into account that Devfile should be returned as part of WorkspaceDto.

I'm not sure it won't cause trouble for other types of entities...

@metlos Can you elaborate more about your thoughts about possible issues? Are you OK with this approach if everything works fine (ci-test result is 97+%, I do not expect issues in master)?

Well, the thing that changes for the clients is that where they saw empty collections coming from our API endpoints, they're going to start seeing nulls. So if anyone happens to rely on non-null arrays being present, they will be caught by surprise by this...

in case of objects, instead of properties with empty arrays, they won't see the properties at all anymore, so the clients will have to start dealing with undefined values where before they didn't have to. Am I right?

I'm OK with making such change because we kinda need it, I just want to point out the consequences for the existing clients (e.g. che-theia and any other custom UI built on top of wsmaster API).

so the clients will have to start dealing with undefined values where before they didn't have to. Am I right?

Right!

@mkuznyetsov and I found some generated typescript DTOs impl which look like initialize collections and maps

home/michael/IdeaProjects/che/dockerfiles/lib/src/api/dto/che-dto.ts export module org.eclipse.che.api.workspace.shared.dto.devfile { export class DevfileDtoImpl implements org.eclipse.che.api.workspace.shared.dto.devfile.DevfileDto { components : Array<org.eclipse.che.api.workspace.shared.dto.devfile.ComponentDto>; metadata : org.eclipse.che.api.workspace.shared.dto.devfile.MetadataDto; projects : Array<org.eclipse.che.api.workspace.shared.dto.devfile.ProjectDto>; apiVersion : string; name : string; attributes : Map<string,string>; commands : Array<org.eclipse.che.api.workspace.shared.dto.devfile.DevfileCommandDto>; __jsonObject : any; constructor(__jsonObject?: any) { this.__jsonObject = __jsonObject; this.components = new Array<org.eclipse.che.api.workspace.shared.dto.devfile.ComponentDto>(); if (__jsonObject) { if (__jsonObject.components) { __jsonObject.components.forEach((item) => { this.components.push(new org.eclipse.che.api.workspace.shared.dto.devfile.ComponentDtoImpl(item)); }); } }

@mmorhun helps us to find that Theia works with DTOs without impls but via generated interfaces, but undefined/null should not happen here since all fields are marked as optional (?) and strictNull option is set on compiler, everyplace should contain the corresponding check before invocation of it. Terminology may be not right, but that's what I understand from converstation with @evidolob and @mmorhun

And Dashboard uses manually written interfaces and not all fields are marked as optional
https://github.com/eclipse/che/blob/master/dashboard/src/components/typings/che.d.ts#L340
Also, I do not know what is settings of compiler, if before invocation client should check field existence. @ashumilova @benoitf Can you take a look? For Dashboard it works in the same way as for Theia? If not, can we so them work in the same way?

in dashboard it fails at runtime if try to access workspace.attributes.plugins, when attributes is null.

For typescript, consumers of the DTO should never see null values on collections/map

if field is mandatory then it needs to be empty value, if the field is optional then it can be undefined

if it's not the case, then the TypeScript DTO needs to be enhanced.

in Ann's case, if attributes is optional field then all consumers have to check if it's defined or not but if attributes is mandatory then it should always return an empty value

mkuznyetsov · 2019-06-12T13:39:26Z

ci-test

che-bot · 2019-06-12T16:54:43Z

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

Signed-off-by: Anna Shumilova <ashumilo@redhat.com>

ashumilova · 2019-06-12T20:51:18Z

ci-test

che-bot · 2019-06-13T00:44:01Z

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

SkorikSergey · 2019-06-13T12:28:53Z

ci-test

Signed-off-by: Mykhailo Kuznietsov <mkuznets@redhat.com>

che-bot · 2019-06-13T14:10:38Z

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

mkuznyetsov · 2019-06-13T14:38:39Z

ci-test

sleshchenko

Platform team agreed that there is no necessity to serialize empty collections/fields.
The only bad side of not doing it: clients must check fields existence before using it.

And as Florent said

if field is mandatory then it needs to be empty value, if the field is optional then it can be undefined

In case of Che, we have some required collections/maps in our model which are used on REST API(machines in workspace config), but required collection or map means that at least one value MUST be present and empty collection/map is not valid.

If anybody has strong concerns against this PR then this solution can be rethinking again even after a merging.
But now it looks as the simplest solution for devfile issue and drawbacks looks not critical.

che-bot · 2019-06-13T17:51:45Z

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

SkorikSergey · 2019-06-14T08:24:21Z

Selenium tests execution on Eclipse Che Multiuser on OCP (https://ci.codenvycorp.com/job/che-pullrequests-test-ocp/1861//Selenium_20tests_20report/) doesn't show any regression against this Pull Request.

Signed-off-by: Mykhailo Kuznietsov <mkuznets@redhat.com>

mkuznyetsov · 2019-06-14T14:01:26Z

ci-test

che-bot · 2019-06-14T17:04:49Z

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

skabashnyuk · 2019-06-15T12:03:12Z

ci-test

che-bot · 2019-06-15T15:12:35Z

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

mkuznyetsov · 2019-06-17T12:25:11Z

ci-test

benoitf · 2019-06-17T15:35:31Z

dashboard/src/app/workspaces/workspace-details/workspace-details.controller.ts

@@ -588,8 +588,8 @@ export class WorkspaceDetailsController {
   * Checks whether "plugins" are enabled in workspace config.
   */
  isPluginsEnabled(): boolean {
-    let editor = this.workspaceDetails.config.attributes.editor || '';
-    let plugins = this.workspaceDetails.config.attributes.plugins || '';
+    let editor = this.workspaceDetails.config.attributes ? this.workspaceDetails.config.attributes.editor : '';


shouldn't it be an empty array instead of '' ? (same for the next line)

that whole method may as well be removed since it's not used

removed the method

che-bot · 2019-06-17T15:38:41Z

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

…3515)

Do not serialize DTO fields with empty or null objects and arrays

45f1c49

Signed-off-by: Mykhailo Kuznietsov <mkuznets@redhat.com>

sleshchenko reviewed Jun 11, 2019

View reviewed changes

core/che-core-api-dto/src/main/java/org/eclipse/che/dto/server/NullOrEmptyMapAdapter.java Outdated Show resolved Hide resolved

sleshchenko reviewed Jun 11, 2019

View reviewed changes

fixup! Do not serialize DTO fields with empty or null objects and arrays

88d4d68

Signed-off-by: Mykhailo Kuznietsov <mkuznets@redhat.com>

mkuznyetsov marked this pull request as ready for review June 12, 2019 07:55

mkuznyetsov requested a review from skabashnyuk as a code owner June 12, 2019 07:55

Merge branch 'master' of github.com:eclipse/che into dto-fix

37a8f23

metlos reviewed Jun 12, 2019

View reviewed changes

Merge branch 'master' of github.com:eclipse/che into dto-fix

13425f1

Fix the usage of empty collections on dashboard

3921fa0

Signed-off-by: Anna Shumilova <ashumilo@redhat.com>

mkuznyetsov requested a review from benoitf as a code owner June 12, 2019 20:35

fixup! Fix the usage of empty collections on dashboard

87ac305

Signed-off-by: Mykhailo Kuznietsov <mkuznets@redhat.com>

mkuznyetsov requested a review from vparfonov as a code owner June 13, 2019 13:40

Merge branch 'master' of github.com:eclipse/che into dto-fix

cb6833c

Signed-off-by: Mykhailo Kuznietsov <mkuznets@redhat.com>

sleshchenko approved these changes Jun 13, 2019

View reviewed changes

Merge branch 'master' of github.com:eclipse/che into dto-fix

d064e1d

skabashnyuk approved these changes Jun 14, 2019

View reviewed changes

Add env variable switch

531ea20

Signed-off-by: Mykhailo Kuznietsov <mkuznets@redhat.com>

fixup! Add env variable switch

cb7d528

Signed-off-by: Mykhailo Kuznietsov <mkuznets@redhat.com>

benoitf approved these changes Jun 17, 2019

View reviewed changes

benoitf reviewed Jun 17, 2019

View reviewed changes

fixup! Fix the usage of empty collections on dashboard

b17b1c5

mkuznyetsov merged commit 84df6f9 into master Jun 18, 2019

mkuznyetsov deleted the dto-fix branch June 18, 2019 10:10

che-bot added this to the 7.0.0 milestone Jun 27, 2019

skabashnyuk pushed a commit that referenced this pull request Jan 3, 2020

Do not serialize DTO fields with empty or null objects and arrays (#1…

b497ebb

…3515)

skabashnyuk mentioned this pull request May 4, 2021

factory resolver misses volume component from devfile 2.0 #19563

Closed

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not serialize DTO fields with empty or null objects and arrays #13515

Do not serialize DTO fields with empty or null objects and arrays #13515

mkuznyetsov commented Jun 11, 2019 •

edited

Loading

todo bot commented Jun 11, 2019

sleshchenko left a comment

mkuznyetsov commented Jun 11, 2019

che-bot commented Jun 11, 2019

metlos Jun 12, 2019

sleshchenko Jun 12, 2019

metlos Jun 12, 2019

metlos Jun 12, 2019

metlos Jun 12, 2019

sleshchenko Jun 12, 2019

sleshchenko Jun 12, 2019

ashumilova Jun 12, 2019

benoitf Jun 13, 2019

mkuznyetsov commented Jun 12, 2019

che-bot commented Jun 12, 2019

ashumilova commented Jun 12, 2019

che-bot commented Jun 13, 2019

SkorikSergey commented Jun 13, 2019

che-bot commented Jun 13, 2019

mkuznyetsov commented Jun 13, 2019

sleshchenko left a comment

che-bot commented Jun 13, 2019

SkorikSergey commented Jun 14, 2019

mkuznyetsov commented Jun 14, 2019

che-bot commented Jun 14, 2019

skabashnyuk commented Jun 15, 2019

che-bot commented Jun 15, 2019

mkuznyetsov commented Jun 17, 2019

benoitf Jun 17, 2019

mkuznyetsov Jun 18, 2019

ashumilova Jun 18, 2019

che-bot commented Jun 17, 2019

Do not serialize DTO fields with empty or null objects and arrays #13515

Do not serialize DTO fields with empty or null objects and arrays #13515

Conversation

mkuznyetsov commented Jun 11, 2019 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

todo bot commented Jun 11, 2019

fix

This comment was generated by todo based on a TODO comment in 45f1c49 in #13515. cc @eclipse.

sleshchenko left a comment

Choose a reason for hiding this comment

mkuznyetsov commented Jun 11, 2019

che-bot commented Jun 11, 2019

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

mkuznyetsov commented Jun 12, 2019

che-bot commented Jun 12, 2019

ashumilova commented Jun 12, 2019

che-bot commented Jun 13, 2019

SkorikSergey commented Jun 13, 2019

che-bot commented Jun 13, 2019

mkuznyetsov commented Jun 13, 2019

sleshchenko left a comment

Choose a reason for hiding this comment

che-bot commented Jun 13, 2019

SkorikSergey commented Jun 14, 2019

mkuznyetsov commented Jun 14, 2019

che-bot commented Jun 14, 2019

skabashnyuk commented Jun 15, 2019

che-bot commented Jun 15, 2019

mkuznyetsov commented Jun 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

che-bot commented Jun 17, 2019

mkuznyetsov commented Jun 11, 2019 •

edited

Loading

This comment was generated by todo based on a `TODO` comment in `45f1c49` in #13515. cc @eclipse.