fix bug in JSONCast

[BSN4]

PR #754 introduced a bug where json string never being loaded ( converted to dict ) .. this PR fix it

[circulon]

PR #754 introduced a bug where json string never being loaded ( converted to dict ) .. this PR fix it

@if4lcon This PR actually reverts to the previous incorrect behaviour where casting to JSON could return an object which is not JSON compatible.

example of previous behaviour which this PR will revert to

class Example(Model):
    __casts__ = {
       "good_cast": "json",
       "bad_cast": "json",
    }

example = Example.hydrate(
  {
    "good_cast": {"this": "dict", "is": "usable", "as", "json"},
    "bad_cast": {'this': 'dict', 'is': 'invalid', 'as', 'json'},
  }

# With your PR the "bad_cast"  will throw an error as it is not valid json even though it is valid python

Can you please provide an issue which illustrates what you are expecting?

[BSN4]

when returning a model response through controller you won't get valid json

[BSN4]

@circulon I made some changes I hope now it fixes your issue and mine too 😀 . can you review it please ?

[josephmancuso]

Looks good to me. Before we break @circulon issue again do you see anything?

[circulon]

Looks good to me. Before we break @circulon issue again do you see anything?

@if4lcon looks like there is the test_model_can_cast_dict_attributes test which has the json.loads() for the payload needs adjusting slightly like the list test

But other than that it looks ok .... nice work @josephmancuso

Do we need tests for the db engines making sure that they will accept the cast data field?

[BSN4]

@circulon done! , all tests passes also I've added a test with the example that you provided

[circulon]

@circulon done! , all tests passes also I've added a test with the example that you provided

I see what your doing with the json cast specific test. it would be more obvious if you moved any payload tests from other test methods into the new json cast test you have added.

In the new test you have added the valid_cast2 has no cast assigned to that field in the model and is not tested.

I just realised that I also did not add a test when setting the payload field with a valid and invalid json string examples setting payload to '{"valid": "json", "int": 1}'- should return a dict setting payload to "{'this': 'should', 'throw': 'error'}" - should fail and assert ValueError I think can you add somthing like these tests within your new test method please?

Good work on getting the bug fixed too... cheers

[BSN4]

setting payload to "{'this': 'should', 'throw': 'error'}" - should fail and assert ValueError I think

I think best way to handle this is to cast it to None and throw an error if you try to set wrong json string.. thats how rails and laravel do it

setting payload to '{"valid": "json", "int": 1}'- should return a dict

this already done.

[circulon]

setting payload to "{'this': 'should', 'throw': 'error'}" - should fail and assert ValueError I think

I think best way to handle this is to cast it to None and throw an error if you try to set wrong json string.. thats how rails and laravel do it

As I understand it the "Casts" are middle men which handle the data on it way into and out of the model. So returning a None makes no sense as the Error would be triggered first. @josephmancuso can confirm/deny this

so throwing an error (which is already done) in the set should be tested for too. From the looks of it you are only testing valid scenarios

setting payload to '{"valid": "json", "int": 1}'- should return a dict

this already done. Yeah sorry I missed that

[circulon]

when returning a model response through controller you won't get valid json

I'm not sure what you mean by this? Can you please provide a small bit of example code please?

[BSN4]

when returning a model response through controller you won't get valid json

I'm not sure what you mean by this? Can you please provide a small bit of example code please?

Lets say you have a get route :

 Route.get("/", "HomeController@index")

HomeController@index:

 def index(self):
    post = Post.find(1)
    return post.payload

you will get invalid json string with slashes .... because it's being json dumped twice

so throwing an error (which is already done) in the set should be tested for too.

I added a test for it you can check last commit

[BSN4]

As I understand it the "Casts" are middle men which handle the data on it way into and out of the model. So returning a None makes no sense as the Error would be triggered first. @josephmancuso can confirm/deny this

I'm copying other frameworks approach here .. I think throwing an error when getting invalid json field would cause issues in future if you're db is corrupted somehow you would think something is wrong with your code where the real problem here is the database. I don't mind both approaches but seems to me None would make you aware of something is wrong without taking your whole app down

[BSN4]

Also in mysql json columns can't have invalid json

[circulon]

PR #754 introduced a bug where json string never being loaded ( converted to dict ) .. this PR fix it

@if4lcon @josephmancuso This has been nagging at me and I now realise what the issue is. As I noted previously this PR will revert to incorrect behaviour by returning an object that is not a str type.

JSON is a string representation of an object (dict or list) structure so it should always be returned as a string.
The JSONCast is correct as it stands as it does the same thing as all the other cast classes. ie transforms an object to a single specific form, in this case a string representation of the object.

As noted in #754 the returned value of a field using this cast should only be a string as that is the valid insertion type that the db engines will expect. Trying to insert a list or dict into a column that has a json datatype will result in an sql error being thrown.

Happy to discuss this further cheers

[BSN4]

PR #754 introduced a bug where json string never being loaded ( converted to dict ) .. this PR fix it

@if4lcon @josephmancuso This has been nagging at me and I now realise what the issue is. As I noted previously this PR will revert to incorrect behaviour by returning an object that is not a str type.

JSON is a string representation of an object (dict or list) structure so it should always be returned as a string. The JSONCast is correct as it stands as it does the same thing as all the other cast classes. ie transforms an object to a single specific form, in this case a string representation of the object.

As noted in #754 the returned value of a field using this cast should only be a string as that is the valid insertion type that the db engines will expect. Trying to insert a list or dict into a column that has a json datatype will result in an sql error being thrown.

Happy to discuss this further cheers

@circulon

I don't understand how you reached this conclusion but thats not how this PR works . you see we only need a json string in set not get. and when the framework fetch back data it can cast it to dict or list through get .

if you have an example please share it so I can test it

[circulon]

I don't understand how you reached this conclusion but thats not how this PR works . you see we only need a json string in set not get. and when the framework fetch back data it can cast it to dict or list through get .

@if4lcon Thats where you have made an assumption about the returned type.
When the value is set if gets transformed to a json compatible string. when the value is returned via get it should be a string which is as it was cast to not as its original representation type.

Like I said this PR will cause an issue when creating records by trying to insert a possibly incorrect object type into a column that is of a JSON type.

the Model.create() method illustrates this

# in the Model.create method there is this login
   if cast == True:
      d.update({x: cls._set_casted_value(x, dictionary[x])})
   else:
      d.update({x: dictionary[x]})

This illustrates that the value will be added as its cast value not its original value. which is exactly where the get method is used to make sure the inserted value is of the correct form.

A Cast column/field should always return its specific representation type otherwise whats the point of casting it in the first place? As we know the column/field is cast to a specific type we can then deal with it appropriately. This PR could return any one of str, dict, list, possibly even an instance of an object where only the str type is db engine compatible.

I hope this clarifies things for you

[BSN4]

@circulon cast not working with model.update() it works with model.save() this is a known issue @josephmancuso working on it in other branch

[circulon]

@circulon cast not working with model.update() it works with model.save() this is a known issue @josephmancuso working on it in other branch

Thanks I had forgotten about that one ;) I submitted that PR ages ago but it was a breaking change that could only go into the next major version unfortunately