New feature: allow nested attributes (#70)

[jcampbell]

Modifies create_object and update_object to allow attributes defined by fields.List and fields.Nested, with new tests and example.

[coveralls]

Coverage increased (+0.1%) to 94.488% when pulling 8fd77ab on jcampbell:feature/list_attributes into 6ab02ab on miLibris:master.

[hellupline]

It was required to change the codebase for flask-rest-jsonapi to allow List Schema ?

I did it with a simple Nested(OtherSchema, many=True) on my schema ( using it on JSON Columns, not relationships)

also, I had issues using List(Nested(OtherSchema)) in the past, then I found about the many=True, (I dont remember what was the problem tho)

[jcampbell]

I'm certainly may have missed a simpler solution and would love to use it...do you have a working example using the Nested(OtherSchema, many=True) approach?

For me, that works fine for reading existing values, but does not allow posting/patching new values. It looked to me like that was because of the way the new object is created in alchemy.py--if the schema declares the field as a Relationship than it is treated specially and the related schema object is created, but otherwise it tries to build the object directly by just unpacking the arguments.

[hellupline]

Indeed nested or list would not create related objects, they would simply set the value into the model, to create related objects, in jsonapi is creating the object on another endpoint and then link then on a relationship endpoint

[jcampbell]

Once again, the point here is not to created a related object, it is to allow complex attributes. The JSONAPI spec is clear that this should be allowed, but currently this library doesn't seem to allow it.

From the JSONAPI spec: http://jsonapi.org/format/#document-resource-object-attributes

Attributes may contain any valid JSON value.

Complex data structures involving JSON objects and arrays are allowed as attribute values. However, any object that constitutes or is contained in an attribute MUST NOT contain a relationships or links member, as those members are reserved by this specification for future use.

The intent of this PR is to allow the use of the Nested fields type to easily allow complex attributes. Security attributes/tags are an example of when one might want to have such complex attributes: for example when we want to manage a list of key/value pairs that define access rules, but that should not themselves ever be addressed as independent objects.

[aleszoulek]

I'm +1 on this PR.

Otoh, if there's currently a good way, how to create one "parent" object with multiple new related objects in one POST, it would be great. Is it possible? If that so, it should be documented somewhere, or at least small example here would be extremely useful.

[hellupline]

I took some time to test this:
your patch breaks the case if a field is a JSON data

{
  "errors": [
    {
      "detail": "Object creation error: (sqlite3.InterfaceError) Error binding parameter 4 - probably unsupported type. [SQL: 'INSERT INTO person (name, email, birth_date, password, test_field) VALUES (?, ?, ?, ?, ?)'] [parameters: ('Pessoa', None, None, None, {'key': 'hello', 'value': '123'})]", 
      "source": {
        "pointer": "/data"
      }, 
      "status": 500, 
      "title": "Unknown error"
    }
  ], 
  "jsonapi": {
    "version": "1.0"
  }
}

a workaround is to create a setter property, which can create your related models, let the model handle the inline attribute

[coveralls]

Coverage decreased (-0.02%) to 94.286% when pulling fe6af03 on jcampbell:feature/list_attributes into ac83e68 on miLibris:master.

[coveralls]

Coverage decreased (-0.02%) to 94.286% when pulling fe6af03 on jcampbell:feature/list_attributes into ac83e68 on miLibris:master.

[coveralls]

Coverage increased (+0.07%) to 94.372% when pulling 9533da6 on jcampbell:feature/list_attributes into ac83e68 on miLibris:master.

[jcampbell]

@hellupline : thanks for flagging...I had not directly supported non-list-like elements originally, but I have updated the PR to support that case as well (you're right, it should, per the spec).

[hellupline]

@jcampbell , atually, the object I used was a list, but the field was a JSON field,

your patch is enforcing to use a DB relationships for nested data ( in your example, Person_Tag model )

you should use sqlalchemy inspect to be sure this is a relationship field

[jcampbell]

It would help me if you could describe what you're expecting or concerned about...it seems correct to me to expect the relationship in the model schema, since that is what is implied by a Marshmallow Nested field. If you'd like a different approach, could you elaborate a bit more?

If you simply used a JSON field type, you would not be using nested at all (and further, that would be making a strong assumption about the DB backing behind SQLAlchemy since that isn't part of SQL directly).

[hellupline]

@jcampbell , my concern is:

Marshmallow Nested Schemas are not "related data", just nested data

Example:

class Address(Schema):
    zip_code = String(required=True)
    country = String(missing='Brazil')
    state = String(required=True)
    city = String(required=True)
    district = String(required=True)
    street = String(required=True)
    number = String(required=True)
    complement = String(missing='')

class Person(Schema):
    address_list = Nested(Address, many=True)

class PersonModel(ModelBase):
    address_list = Column(JSON(), default='[]', nullable=False, server_default='[]')

in this example, there is no need "address_list" to be a second table, its a nested data in the PersonModel table

my concern is that your patch could interpret the input of this schema as sqlalchemy relationship.

I do agreed that be able to create nested relationships is nice ( I even have a perfect case for this )
but this new feature must be iron solid, to avoid undesired behaviours, should only affect relationships and I think it would be nice if I could optout this featured on some Resources ( maybe as arg for the data_layer )

[coveralls]

Coverage decreased (-0.07%) to 94.239% when pulling 23eceee on jcampbell:feature/list_attributes into ac83e68 on miLibris:master.

[jcampbell]

@hellupline I've updated the patch to use inspection as you suggested, which should address the case you identified.

It's a bit tricky to fully test with the current unit testing suite since JSON isn't a standard SQL type. I've tested it locally using postgres backing to ensure that both serialization/deserialization and marshmallow schema validation work.

[hellupline]

@jcampbell , indeed SQLAlchemy is yet to add JSON column type to sqlite

[sodre]

@hellupline, any chance we can get this PR merged? It seems that a .07% decrease in coverage for adding 1 lines seems acceptable when it is making the library more compliant with the spec.

[hellupline]

@sodre I am not the owner of the repo, just a annoying dev with strict code quality rules ( I do code review ).
I dont see a problem merging this

@akira-dev what do you think, its acceptable ?

[sodre]

@akira-dev, ping 👍

[sodre]

@akira-dev, any change you can take a look at this PR? It is about 1-year old now.

[akira-dev]

Just sorry I will merge this pr today or tomorrow thanks for your work

[akira-dev]

Thanks a lot for your work and sorry for the delay