pivotModel stored within a proxy wrapper for a relatedModel, accessible trough a virtual property as configured by the 'pivotKey'

[adm-bome]

Actively look-up pivot data on models

Type of PR:

• [x] Bugfix
• [x] Feature
• [ ] Code style update
• [x] Refactor
• [ ] Build-related changes
• [ ] Documentation
• [ ] Other, please describe:

Breaking changes:

• [x] No
• [ ] Yes

Details

Bug Pivot data, can and 'almost' always, is different for the related relation to the model. (why else have pivot data). When models have an relation to the a related Model, Pivot data should not be stored on the related model, because the related model is a object by reference the pivot information gets overwritten each time the mapper sets the pivot record to the related model. only the last mapping is remembered.

Solution The related model should be a proxy between the related model and it pivot record. Pivot on the proxy must be a 'virtual' property.

Feature Possibility to have multiple models with a relation to the same related model with different pivotData.

Refactor from the pivotKey in a model, to an Proxy with an 'locally/virtual' stored reference to the pivot data.

Issue: #668

[codecov[bot]]

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@b5c6f1e). Click here to learn what that means. The diff coverage is 59.45%.

@@            Coverage Diff            @@
##             master     #669   +/-   ##
=========================================
  Coverage          ?   98.99%           
=========================================
  Files             ?       48           
  Lines             ?     1585           
  Branches          ?      268           
=========================================
  Hits              ?     1569           
  Misses            ?       16           
  Partials          ?        0           

Impacted Files	Coverage Δ
src/model/Serialize.ts	90.47% <40.00%> (ø)
src/model/proxies/ModelProxies.ts	53.84% <53.84%> (ø)
src/attributes/relations/BelongsToMany.ts	100.00% <100.00%> (ø)
src/attributes/relations/MorphToMany.ts	100.00% <100.00%> (ø)
src/attributes/relations/MorphedByMany.ts	100.00% <100.00%> (ø)
src/query/filters/LimitFilter.ts	100.00% <0.00%> (ø)
src/attributes/types/Attr.ts	100.00% <0.00%> (ø)
src/schema/IdAttribute.ts	100.00% <0.00%> (ø)
src/attributes/relations/HasManyThrough.ts	100.00% <0.00%> (ø)
src/plugins/use.ts	100.00% <0.00%> (ø)
... and 43 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b5c6f1e...fa56290. Read the comment docs.

[cuebit]

Unfortunately Proxy is not supported in IE and this would introduce breaking changes beyond the library's support scope.

I think we need to revise this approach without the use of Proxies, if at all possible. Perhaps we can look into hydration of models without reference?

[Tofandel]

IE is completely EOL (all IE even IE 11) and even microsoft doesn't support them anymore (https://techcommunity.microsoft.com/t5/microsoft-365-blog/microsoft-365-apps-say-farewell-to-internet-explorer-11-and/ba-p/1591666)

It always possible to use this polyfill which doesn't cover all the Proxy class features because it's impossible but covers some simple use cases (which is the only thing this PR uses)

Otherwise it would be possible to remove pivot completely and instead introduce a pivots(related_id = null) method which would return all the pivots if no related_id and only the pivot of the related_id if given which would take care of the bug

Hydration of models without reference would introduce an even bigger breaking change (Because then === doesn't work)

This is a very important issue and I believe the way to go for now is proxy which can be poly filled enough to support this PR

#602, #623

[cuebit]

The main concern is breaking the support paradigm in Vue 2. Vuex ORM was initially and will continue to be adopted with Vue 2 in mind which is the primary reason Proxy is not the preferred choice here (else it would make a lot of things easier).

That being said, IE is not necessarily the driving factor in this decision hence why it won't be a topic worth entertaining here. What's more important is the reasoning for simulating a technology to solve a problem that can be solved in a more amicable fashion without hindering the line of support that already exists.

Hydration of models without reference would introduce an even bigger breaking change (Because then === doesn't work)

I'm not sure I follow. The key issue with pivots is that they are attached by reference. This is not an issue for 1:1, 1:m or m:1 relations because pivots can only bridge a child relation once. However, m:m relations can share the same child – and since they are preloaded, attached, then enumerated to update pivot data exclusively, the reference becomes an issue.

---

One solution could be to rehydrate models before attaching. This is not exactly ideal in a benchmark competition because in almost all instances models are hydrated when queried (even internally) anyway. Re-hydration inevitably adds to the N^th degree. However, it doesn't require polyfilling, doesn't introduce breaking changes since it's out of the public scope, and doesn't impact support coverage for projects with existing targets. Does this make sense?

[Tofandel]

So the problem is the not that the pivot is attached by reference but rather the entire model, so when you do a query, you will always get the same reference to your model, and this is what is causing the issue. Because whenever you fetch a model with a different pivot, the pivot is overwritten on all the models because of the reference

So actually, looking at the PR, yes the proxy is not needed, since it's anyways creating a new instance of the PivotModel proxy,

It could just be replaced by m = new model(attributes)

But in both cases you'd have a small breaking change, because now many to many models accessed through relations will now never have the same reference because they are new instances on every query fetch

Some tests related to memory should be done as well, to make sure that it won't cause a memory leak

[cuebit]

Hah, quite right. Indeed I was referring to the model instances the pivots are attached to. Nonetheless, I think it’s safe to say we’re seeing the same picture now.

Regarding object equality, I think for a m:m relations this would be quite difficult to maintain without substantial refactoring, particularly since a reference to hold existing models to prevent the breaking change would indeed lead to memory leaks. As mentioned in earlier issues you’ve referenced, the test suites didn’t cover the more complex scenarios so this was missed during development.

Of course, we welcome PRs that resolve existing issues and this one has been a long standing issue.