iknow / iknow_view_models Goto Github PK

(1) it might be nicer for viewmodel edit checks to deal in terms of viewmodel’s attributes and association’s rather than model’s.

To do this, we could use the attribute_changed! during deserialization that’s used in VM::Record.

(2) This is awkward to do, because we have limitations on how we can edit check associations to children, when permissions vary depending on what you’ve done to the child.

For example, if we have Lessons and LessonIssues. There are CREATE, EDIT, VIEW permissions for LessonIssue. Anyone who has CREATE or EDIT is allowed to create a LessonIssue (and attach it to a lesson)

However, someone with CREATE shouldn’t be able to move a LessonIssue from one lesson to another.

However, currently, this is only enforced by the lesson_id model attribute change in the LessonIssue (i.e. to change attributes on an existing issue, you have to have EDIT)

But if lesson_id were no longer present in the LessonIssue changes, we’d have to enforce the change from the Lesson side.

So the Lesson side would have to know whether the issue that’s being attached to it is new (in which case allow it with CREATE) or pre-existing (in which case allow it with EDIT)

In this specific case it’s potentially simpler, because LessonIssues must have a lesson. Because they can’t previously be unparented, the edit check in Lesson could be presented with the information about whether the changed associated records are being attached or detached from the Lesson. The attach could be allowed to everybody, but the detach could require edit. That’s enough to prevent a CREATEor from moving a lesson from another parent.

However, if LessonIssues were allowed to have null parents, then “moving” one from no parent to a parent would still only be an attach, and therefore couldn’t be prevented by the parent. (unless the parent had a view into the content of the child, which we would want to avoid)

However, maybe this is still all consistent.
If we claim that associations are always owned 100% in the down direction, then attaching a lesson issue to a lesson is not a property of the lessonissue, but of the lesson.
and we’re therefore required to come up with a permissions scheme that makes sense in that context.

When we touch an entity we want to include our effects in our resulting serialised value. This is implemented by propagating the value of UpdateData#updated_associations to the pruning of the serialisation context.

This functionality is simply missing for entities that are modified through a shared relationship. DeserializeContext#updated_associations is populated from the root only. The SerializeContext#serialize_references method uses self as the context for the referenced updates, and leaves no opportunity to use pruning other than the root pruning.

The callback methods on viewmodel classes are frequently used to modify the model. In order that access control has a chance to inspect those changes, that means they should be run before the access control callback is invoked, with other updates_view callbacks.

Presently, this doesn't happen - they're run at the end:

Our current strategy of shallow changes and delegating to ActiveModel::Dirty leaves some holes in things that we can check. We'd instead like to build a deep structure, bottom-up, with an interface something like

changes.for_attribute(:attr)
=> [old,new] | nil  # delegate to AR::Dirty, plus or minus any `format`?
changes.for_association(:single_association)
=> Changes | nil
changes.for_association(:collection_association)
=> { added: { child_view => Changes[] }, removed: { child_view => Changes[] }, edited: { child_view => Changes[]} }

This could be useful in cases where we entities that can provide substantially the same view type spread across two different models. Consider the new content system: we have both Sentence and LocalSentence, and both Word and LocalWord. Both the global and local forms have translations etc, but unless we want to deal with polymorphic associations they need to be in separate tables. It might be nice for the views to abstract over this structural difference that's only necessary for storage.

In principle we could specify the possible models for the viewmodel as a hash from parent view to model class - something like { WordView => WordTranslation, LocalWordView => LocalWordTranslation }.

We could then use deserialization parent context to select the appropriate model to deserialize a new instance into.

https://github.com/iknow/iknow_view_models/blob/master/lib/view_model/active_record.rb#L307

https://github.com/iknow/iknow_view_models/blob/master/lib/view_model/active_record/update_context.rb#L162

So database errors such as foreign key violations will bubble out unwrapped. I think we don't want this.

There's a single context for all reference serialisations (via SerializeContext#for_references), which prevents specifying prune or include. We'd have to allow the prune and include to be parameterised by type. This could be in terms of a prune/include tree by root type, or a flattened context-free specification per type. While we're here, make sure that the changed_association propagation works with references.

When a subtree is deleted, only the root is put into the release pool. This means that if a child node of a deleted subtree is added elsewhere, it will not be resolved from the release pool but instead newly fetched from the database. This newly created update operation conflicts with the delete and fails.

We often inspect changes in access control definitions as values, which means we can mix strings and symbols, and also have unchecked typos render policies ineffective.

Define an API for changes so that every comparison has the chance to be string/symbol coerced, and checked against the defined properties of that model.

Currently, through: association are implemented with the assumption that they're for use with many-to-many associations, which don't make sense at all to be owned. Because of that, a through: association to a non-root isn't supported. This is a bit over-strict, since there are other valid cases to have an association use a hidden join table, such as an ordered polymorphic collection.

Achieving this will require a hefty refactor of UpdateOperation#build_updates_for_collection_referenced_association to generalize it to work for both referenced and nested targets.

An additional feature that we may want to consider supporting should we refactor this is to support polymorphic associations where the targets may be either (referenced) root types or (nested) owned types. This would allow us to better represent the common vs custom text associations as a single polymorphic association.

If an association doesn't have an inverse set correctly, cache clearing might not work on free. Make sure that all owned associations have an inverse (or, if polymorphic, have an inverse for each of the viewmodel types)

To reduce the scope for unexpected interactions

So that a view can specify a particular customization of its subview.

Shared associations don't really ideally represent the way we use viewmodels. What we've ended up with is viewmodels that are either always or never a root, not dependent on context. Roots are always accessed via indirect references. Orthogonally we have ownership, describing whether a associated model is owned by the parent.

Presently these two concepts are rolled into shared associations, so roots can never be owned and vice versa. This isn't helpful: among other things it means we can never have a view that's owned by one parent and shared by others.

Proposal:

• Remove shared from associations, and define root on the class level of VM::AR: a viewmodel type is either always a root or never a root.
• Fill out the support for having rooted and non-rooted children on any association type - at present has_one and has_many can not be rooted (since they inherently couldn't be shared).
• Add owned (default: true) to associations.
• Add support for cleanup of owned rooted children

We need this. Could implement with a flag in association_data, and check when building that new child viewmodels are the same as previous child viewmodels.

The following upload should create and return a single section. After the preload step, the same section is returned twice.

Speculation: The preloader examines the first thing in the collection to determine if it should load everything. Since the first entity's association(:sections).loaded? is false, it will load that layer of the tree, including the second entity. Since we've already set the association(:sections).target value as part of the update, the value is incorrectly updated to include the preloader results, which duplicates the contents of sections.

Request:

{
    "data": [
        {
            "_type": "Exercise"
        },
        {
            "_type": "Exercise",
            "sections": [
                {
                    "_type": "Section",
                    "section_type": "Simple",
                    "visibility": "All"
                }
            ]
        }
    ]
}

Response (abbreviated):

{
    "data": [
        {
            "_type": "Exercise",
            "id": 157,
            "sections": []
        },
        {
            "_type": "Exercise",
            "id": 158,
            "sections": [
                {
                    "_type": "Section",
                    "id": 141,
                    "section_type": "Simple",
                    "visibility": "All"
                },
                {
                    "_type": "Section",
                    "id": 141,
                    "section_type": "Simple",
                    "visibility": "All"
                }
            ]
        }
    ]
}

If we didn’t make changes to the model, UpdateContext doesn’t call save. If it doesn’t call save, it doesn’t clobber previous_changes on the model, so previous_changes that didn’t come from deserialization are available for the access control to look at, without much way to distinguish them.

This isn't very nice. Consider if visiting a model for deserialization should clear its previous changes regardless of whether it saves or not, thus ensuring that previous_changes throughout the tree is guaranteed to exactly represent the changes made to the tree.

We currently require that a type field refer to exactly one view model, which requires view models with polymorphic elements to use a secondary type field. If we allowed view models to register under aliases, they could register their subtypes and use the regular type field.

Only the deleted node is visited - we made the assumption (intentionally) that having permissions to delete a parent implies necessarily having permissions to delete its wholly owned children, even if the permission is not explicitly stated.

However, it's no longer valid to make this assumption in the case of generalized viewmodel callbacks: an on-change hook on a node should still have a chance to have its side effects even if the node was deleted recursively via its parent.

So, we're going to have to revisit subtree deletion. The implementation of this should take into account #8

Because they aren't checked until commit, which is potentially outside the library. To handle this, we should call SET CONSTRAINTS ALL IMMEDIATE at the end of deserialization to ensure constraints are checked with appropriate handling before we return to the application.

for DeserializeContext, in order to encapsulate changed_associations, changed_attributes and deleted and provide common behaviour.

Defining lifecycle hooks as regular Ruby methods carries the risk of clobbering inherited lifecycle behaviour. Consider instead using the ViewModel::Callbacks hook DSL.

Consider supporting "reference-only" associations. This could help reduce the need for defining special id-only views.

VM::AR.load and .find both support a parameter lock: to specify pessimistic locking. We'd like this on deserialize as well. This will mean passing the locking parameter into UpdateContext to be used when preloading/loading models.

Relatedly it'd be nice to update VM::AR::Controller to have lock: keyword arguments on its show, index and create actions to allow the behaviour to be easily changed by overriding and calling super with arguments.