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

Feature: new ignoreRequiredCheck flag, and serialize objects using specified class mappings #126

Merged
merged 2 commits into from
Apr 13, 2020

Conversation

cmolodo
Copy link
Contributor

@cmolodo cmolodo commented Mar 3, 2020

Reopening a pull request similar to #92 using a rebased branch.

This includes 2 new features:

  1. New ignoreRequiredCheck flag which effectively makes all properties optional, even if the property mapping doesn't specify isOptional = true. (commit 85747a8)
  2. Added optional classReference parameters to all serialization methods, to allow serialization of an object using mappings from the specified class reference instead of mappings from the object itself. (commit 0a044c2)

These are both features needed to support my specific use case. My data is represented by a nested structure of typescript classes with JSON mapping annotations. This data is filled in using Angular forms, which output a mix of pure JSON objects and typescript class objects, depending on the control values - I want to be able to send this data to the back end directly using my pre-defined mapping annotations. Additionally, because the data structure is large, users need to be able to edit portions of the data, and I want to serialize these patch objects without triggering an error due to other required properties not being present.

So these new features for me solve 2 problems:

  1. By serializing an object using mappings defined on another class, I can easily serialize my form output to send to the back end without having to do a lot of additional logic or data management.
  2. By skipping the required property check, I can serialize partial (patch) objects that match my defined class without errors, but still enforce the requirements when deserializing.

As a trivial example, say I have the classes:

// Dictionary class, not editable by user
@JsonObject("Country")
export class Country {
    @JsonProperty("countryName", String)
    name: string = undefined;
}

@JsonObject("Parent")
export class Person {
    @JsonProperty("name", String)
    name: string = undefined;
    @JsonProperty("country", Country)
    country: Country = undefined;
}

Together, these features let me serialize a patch edit object of the parent where I change the country without affecting the name property, and let me use the mappings defined on my classes to do so:

const formOutput = {
    country: {
        name: "Some Country"
    }
};
// No errors will be thrown, and the output JSON will have the country.name property
// mapped as country.countryName.
const json = jsonConvert.serializeObject(formOutput, Person);

cmolodo added 2 commits March 3, 2020 12:41
…d" property check being skipped, so all mapped properties become optional. Added unit and integration tests of new flag.
…present will be used to provide mappings. (If not present, uses mappings on the provided data object.)
@belavenuto
Copy link

Any prediction of when this PR will be promoted to master?

@andreas-aeschlimann
Copy link
Member

Please allow me to look through it closely, but it should be within the next days.

@sagunpandey
Copy link

@andreas-aeschlimann Hi Andreas, apologies to bug you at a time like this. Is this any update on when this is going to the main release. Our project completely relies on this library. And not being able to serialize objects that weren't created using new operator is breaking everything. Thank you for your kind assistance.

@jpabeem
Copy link

jpabeem commented Apr 11, 2020

Agree with @sagunpandey here, I almost pulled my hair out as I couldn't figure out what I was doing wrong. Glad I was able to find this issue documented + associated PR.

My use-case involves mapping camelCase properties to all lowercase characters. I create objects with nested objects as fields with the use of TypeScript's Partial<T> and thus I don't use the new operator as well.

I've been able to solve my problem with a custom solution, it just isn't maintenance-friendly in the long term and feels hacky. We would love to use json2typescript for this!

@sagunpandey
Copy link

Agree with @sagunpandey here, I almost pulled my hair out as I couldn't figure out what I was doing wrong. Glad I was able to find this issue documented + associated PR.

My use-case involves mapping camelCase properties to all lowercase characters. I create objects with nested objects as fields with the use of TypeScript's Partial<T> and thus I don't use the new operator as well.

I've been able to solve my problem with a custom solution, it just isn't maintenance-friendly in the long term and feels hacky. We would love to use json2typescript for this!

@jpabeem Can you please share your hacky way of resolution? Unless the library gets fixed, I would want to have some sort of temporary solution.

@andreas-aeschlimann
Copy link
Member

Thank you for your messages. I will do it this weekend, latest this Monday.

Apologies for the delay but I really want to look through it in a quiet minute before publishing it.

@jpabeem
Copy link

jpabeem commented Apr 13, 2020

Agree with @sagunpandey here, I almost pulled my hair out as I couldn't figure out what I was doing wrong. Glad I was able to find this issue documented + associated PR.
My use-case involves mapping camelCase properties to all lowercase characters. I create objects with nested objects as fields with the use of TypeScript's Partial<T> and thus I don't use the new operator as well.
I've been able to solve my problem with a custom solution, it just isn't maintenance-friendly in the long term and feels hacky. We would love to use json2typescript for this!

@jpabeem Can you please share your hacky way of resolution? Unless the library gets fixed, I would want to have some sort of temporary solution.

I am also not sure if it fits your use-case, but I've implemented a flattening solution similar to this. The gist of it comes down to flattening the object to a depth of 1 and replacing the object property names to contain all lower case.

It's not the prettiest and may break things in the future, that's why we would like to use json2typescript w/ the @JsonProperty to make it more explicit that the object' property names before flattening are all lower case.

@andreas-aeschlimann thanks for the quick update, much appreciated. Happy Easter!

@andreas-aeschlimann andreas-aeschlimann merged commit 837b8a0 into appvision-gmbh:master Apr 13, 2020
@andreas-aeschlimann
Copy link
Member

andreas-aeschlimann commented Apr 13, 2020

I was just wondering @cmolodo, why have you chosen any as type for the parameters in the new serialize methods? Shouldn't it be object | object[] instead to disallow primitives and undefined?

Anyway, this pull request has been merged and I published v1.3.0.

I also updated the docs because it was forgotten in the changes.

@sagunpandey
Copy link

@andreas-aeschlimann @cmolodo You guys rock. I will make a few tests and see everything is intact. Thanks again.

@sagunpandey
Copy link

sagunpandey commented Apr 14, 2020

@andreas-aeschlimann After upgrading to v1.3.0, I am getting the following error:

ERROR in ../node_modules/json2typescript/src/json2typescript/json-convert.d.ts:26:9 - error TS1086: An accessor cannot be declared in an ambient context.

26     get operationMode(): number;
           ~~~~~~~~~~~~~
../node_modules/json2typescript/src/json2typescript/json-convert.d.ts:37:9 - error TS1086: An accessor cannot be declared in an ambient context.

37     set operationMode(value: number);
           ~~~~~~~~~~~~~
../node_modules/json2typescript/src/json2typescript/json-convert.d.ts:57:9 - error TS1086: An accessor cannot be declared in an ambient context.

57     get valueCheckingMode(): number;
           ~~~~~~~~~~~~~~~~~
../node_modules/json2typescript/src/json2typescript/json-convert.d.ts:68:9 - error TS1086: An accessor cannot be declared in an ambient context.

68     set valueCheckingMode(value: number);
           ~~~~~~~~~~~~~~~~~
../node_modules/json2typescript/src/json2typescript/json-convert.d.ts:80:9 - error TS1086: An accessor cannot be declared in an ambient context.

80     get ignorePrimitiveChecks(): boolean;
           ~~~~~~~~~~~~~~~~~~~~~
../node_modules/json2typescript/src/json2typescript/json-convert.d.ts:87:9 - error TS1086: An accessor cannot be declared in an ambient context.

87     set ignorePrimitiveChecks(value: boolean);
           ~~~~~~~~~~~~~~~~~~~~~
../node_modules/json2typescript/src/json2typescript/json-convert.d.ts:105:9 - error TS1086: An accessor cannot be declared in an ambient context.

105     get propertyMatchingRule(): number;
            ~~~~~~~~~~~~~~~~~~~~
../node_modules/json2typescript/src/json2typescript/json-convert.d.ts:115:9 - error TS1086: An accessor cannot be declared in an ambient context.

115     set propertyMatchingRule(value: number);
            ~~~~~~~~~~~~~~~~~~~~
../node_modules/json2typescript/src/json2typescript/json-convert.d.ts:131:9 - error TS1086: An accessor cannot be declared in an ambient context.

131     get ignoreRequiredCheck(): boolean;
            ~~~~~~~~~~~~~~~~~~~
../node_modules/json2typescript/src/json2typescript/json-convert.d.ts:140:9 - error TS1086: An accessor cannot be declared in an ambient context.

140     set ignoreRequiredCheck(value: boolean);

Something might have broken. Are you seeing any issue? I am still on Angular 8.0. Could this have been the issue?

UPDATE 1:

For now I added "skipLibCheck": true under compilerOptions and the error stopped showing up.

However,

deserializeObject() method gives be a non-empty object but with all properties equal to null.

Am I missing something here?

@andreas-aeschlimann
Copy link
Member

andreas-aeschlimann commented Apr 14, 2020

@sagunpandey this has nothing to do with the PR, it is caused by the TypeScript update. The only possibility is probably to downgrade in json2typescript itself. Please let's discuss it in a separate issue.

@cmolodo
Copy link
Contributor Author

cmolodo commented Apr 21, 2020

@andreas-aeschlimann Oops sorry I missed your comment earlier! You're right actually, it should have been object/object[] rather than any - to be honest I just didn't think of it. Sorry about that! I can submit a new pull request with that change if you like.

Thanks for accepting this, and for updating the docs!!

@andreas-aeschlimann
Copy link
Member

No worries. If you have time, you can do a PR. Otherwise I will fix it in a later commit.

@andreas-aeschlimann
Copy link
Member

@cmolodo you might find it funny, but today I ran into a case where I thought that we need a third option for the ignoreRequiredCheck.

I have strict mode enabled, but some properties may be null or primitive. When the server sends null for a primitive property, deserialization succeeds because I mark the property optional. However, when I want to serialize, the property is dropped. I rather send null values back to the server so the server knows that I actually want to set the value to null.

We could add a third option for ignoreRequiredCheck which makes sure all fields are forced non-optional whether we have isOptional true or false. It is actually the «opposite» of ignoreRequiredCheck = true.

What do you think?

@andreas-aeschlimann
Copy link
Member

@cmolodo let's continue the discussion about the

@andreas-aeschlimann Oops sorry I missed your comment earlier! You're right actually, it should have been object/object[] rather than any - to be honest I just didn't think of it. Sorry about that! I can submit a new pull request with that change if you like.

Thanks for accepting this, and for updating the docs!!

Let's continue this discussion here: #137

@andreas-aeschlimann
Copy link
Member

@cmolodo you might find it funny, but today I ran into a case where I thought that we need a third option for the ignoreRequiredCheck.

I have strict mode enabled, but some properties may be null or primitive. When the server sends null for a primitive property, deserialization succeeds because I mark the property optional. However, when I want to serialize, the property is dropped. I rather send null values back to the server so the server knows that I actually want to set the value to null.

We could add a third option for ignoreRequiredCheck which makes sure all fields are forced non-optional whether we have isOptional true or false. It is actually the «opposite» of ignoreRequiredCheck = true.

What do you think?

I have a suggestion for this problem. Let's discuss here: #138

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants