fix: allow simple-json columns to hold non-object types (#5116)

[svvac]

No description provided.

[pleerock]

wait, but why we should support a non-json value in a column that supposes to contain a json value?

[svvac]

"some \" string ", true, and 42 are valid JSON value representations that are not of the object type (respectively a string, a boolean and a number).

JSON.serialize() and JSON.parse() recognizes them as they are valid JSON.

A valid use case of mine that gets broken by the regression is storing schemaless data as key-value pairs that support simple types as well as complex array/object types, like for an extensible configuration map (see basic example in #5116)

[pleerock]

Okay, so these changes were made by #4476 (issue #4440). I need feedback from @abhijeet1403 @vlapo @haydennyyy to conclude on a design decision around this feature. Do we want primitives to be handled by simple-json or not. Personally I'm not fan of this approach, but I found that Postgres's json type also does it.

[svvac]

Note that #4476 introduced breaking changes that were not labelled as such and could break existing applications silently.

I understand why some would want the behaviour introduced in that PR, but IMHO it should be made available as an opt-in, either using a new type (e.g. simple-json-object) or a column flag (e.g. jsonCastObject: true).

Also note that the problem outlined in #4440 could have been solved by ad-hoc defaults in the model and proper data validation prior to persisting data in the application, which are not really the responsibility of TypeORM.

[abhijeet1403]

The reason for raising the PR was because for empty strings
The check typeof value === "string" was passing
but JSON.parse("") was failing with

Uncaught SyntaxError: Unexpected end of input
Check issue 4440

Which is not the expected behavior and was breaking systems in case the JSON field was left empty/null.

Its a behaviour of JS itself, checkout this stackoverflow post

So the fix just adds checks to resolve values only if it is resolvable where the JSON field is empty in the database, and that it is being used to store JSON objects.

For example the following test case was failing other wise,

it("should correctly add retrieve simple-json field with no value", () =>
   Promise.all(connections.map(async (connection) => {
       const repo = connection.getRepository(Post);
       const post = new Post();
       post.id = 1;
       post.jsonField = "";
       await repo.save(post);
       const postFound = await repo.findOne(1);
       postFound!.id.should.eql(1);
       postFound!.jsonField.should.eql({});
   })));

If a flexible behavior is intended, wouldn't it be wise to use text as the data type and parse it to JSON as required. After all "simple-json" data type is just text with few utility functions to handle it as simple objects.

In case it is still desirable to store non-object types the (typeof simpleJSON === "object") at the end check can be removed.

[svvac]

post.jsonField = "";

Well in that case, the value persisted in the database would be "" (JSON.stringify('') === '""') and not the empty string. The model should have jsonField: object and typescript should throw when assigning the string.

[svvac]

Regarding the empty column case you were talking about, JSON.parse(null) === null which seems acceptable for a SQL NULL value. Returning null when the column is effectively set to the empty string also seems acceptable.

However, silently replacing invalid JSON values with empty objects sounds rather dangerous and can lead to unwanted and unnoticed data loss. It is even more so for valid « unexpected » values.

[pleerock]

post.jsonField = "";

this sounds like a proper use case for reseting data in the field, but looks like it will be inappropriate use case. we can't normalize it to '""' because it will lead to bugs, and looks like we need to handle this case and throw an exception instead? I strongly don't like simple-json to be non-simple and confusing and thats exactly where we lead it to.

[svvac]

a proper use case for reseting data in the field

Reseting a object is post.jsonField = {} in JS. You can set it to null also.

we can't normalize it to '""' because it will lead to bugs

What bugs specifically? I see the empty string '' problem, but not that one.

I strongly don't like simple-json to be non-simple and confusing and thats exactly where we lead it to.

The simplest I can come-up with is that when you set a simple-json field to a value, it will return that same value when fetched from the database. Everything else is just magic and begging for bugs/confusion.

It sounds to me that the ONLY real problem is the case where the database returns the empty string, which JSON.parse chokes on, and that could be solved by returning null in that case.

• Passing invalid values (e.g. circular structures and whatnot) will get an error from JSON.stringify(). You probably need to take care of the few cases where it returns undefined, but that's about it.
• Bogus persisted values will throw when fetched because of JSON.parse(). I believe this is acceptable since the library cannot do anything with it except dropping it, and it indicates an inconsistency in the model that should be properly handled by e.g. validation.
• When you set the field to something, you get that back afer a roundtrip to the database : no surprise, no magic default, no special invalid values. A plain old field just like in any other JS object.

With those semantics, you leave to the devs the responsibility to properly initialize the value, be consistent in the values they put inside the field, and validate their inputs. And you cannot do that in their stead without making assumptions about their models. This should be properly documented of course, but doesn't sound unreasonable at all

[svvac]

@pleerock so, what do we do?

[pleerock]

I'm waiting to see other people feedback. Since many people already had some problems / fixes in this area maybe they can say something here. I don't want to merge it then get ( as usual:( )many negative feedback about functionality being constantly changed.

[jushar]

I've been running into the same problem as the PR author, because using simple-json as a "generic" key-value store seems natural to me (as stated in #5116).
Therefore, I cannot currently upgrade to the latest typeorm version because of that.

Making it opt-in (see: #5125 (comment)) sounds like a good idea imho.

[abhijeet1403]

@pleerock I think let’s go with this PR.

[abhijeet1403]

@svvac can you add tests and confirm if the test suite is passing ?

[svvac]

@abhijeet1403 Can do but what's the plan? Get rid of object casting altogether?

[abhijeet1403]

I think the proposed changes in this PR looks fine.
i.e we’ll parse and return whatever JSON.parse() supports and empty object in case there’s no value or there’s an error.

[abhijeet1403]

@svvac If you're not updating this MR, should I go ahead, update this branch with tests and get these changes merged?

[imnotjames]

Closing this in favor of #6574