isVirtualAttribute/appendVirtualAttribute are global in v5

[MordantWastrel]

(apologies if this is a duplicate, I tried to create it yesterday from my phone but it doesn't seem to have taken)

writedump(getInstance("RMME_A")
    .isVirtualAttribute("foo")) // false

getInstance("RMME_A")
    .appendVirtualAttribute("foo"); // permanently mutates target binding?

writedump(getInstance("RMME_A")
    .isVirtualAttribute("foo")) // brand new instance, but it's true

[davidAtInleague]

Wondering if this is intentional or not -- I could see that it's possibly some optimization that drives v4->v5 perf improvements, but also it could be an oversight.

We have some code to fixup if this is an intentional behavior, but aren't sure if it needs fixing, or if the breakage is a bug and we should holdout for a bugfix.

Mostly looks like (psuedocode, probably some typos)

function instanceReady() {
    if (isVirtualAttribute("foo")) {
        // This if block used to be entered only if this instance was instantiated
        // with the virtual attr, but now it will always be entered if any
        // instance was ever instantiated with the virtual attr,
        // because virtual attrs are persistent once 'installed'.
        // When a virtual attr is "present by virtue of having been
        // installed at least once during server lifetime, but it's not
        // actually been requested for this instance", it has value `""`.
        ... do something with this.getFoo() ...
    }
}

function scopeWithFoo( qb ) {
    appendVirtualAttribute("foo");
    qb.selectRaw("<aggregate json or something> as foo")
}

[elpete]

This looks like a bug. A virtual attribute metadata entry should not be shared between instances.

[MordantWastrel]

Just to play devil's advocate, this seems like an expense with a very marginal gain.

Given that v5 is very performance-oriented, is the use case for tracking instance-specific virtual attributes worth the squeeze of tracking one more per-instance 'thing'?

When I'm writing Quick code, I am defining a virtual attribute as something that might pop up on this instance depending on whether or not I've asked for it. Typically, when I want to see if an attribute has got something going on, I'm going to do a null check or a length check. Quick right now has its own 'are you a thing' check of isVirtualAttribute().

We use this a lot so I'm arguing against my own interests here, but I wonder whether we really need per-instance metadata about virtual attributes, rather than just 'does this virtual attribute have a value' which is usually what we're actually asking about.

[davidAtInleague]

There might be an inadvertent write into cache:

// modules/quick/models/BaseEntity.cfc:2574#appendVirtualAttribute
variables._meta.attributes[ arguments.name ] = variables._attributes[ arguments.name ];

// but maybe unexpectedly, the above writes into cache (shared w/ all instances)
var x = variables._meta.attributes;
var y = variables._cache.get( "quick-metadata:#variables._mapping#").attributes;
var isLiveRefToCache = x === y; // true

// modules/quick/models/BaseEntity.cfc:2508#metadataInspection
explodeAttributesMetadata( variables._meta.attributes ); // <- pulled from cache

explodeAttributesMetadata then reconstitutes the virtual attributes from cache on each onDiComplete

(callstack is)

explodeAttributesMetadata
metadataInspection
onDiComplete
...

[MordantWastrel]

@elpete To kick-start this issue (which is the last blocker for us on v5 that I can think of):

It seems like virtual attribute data should be cached on the builder, not globally; while one could make an argument that All Of Quick might as well be aware that foo is sometimes an attribute on bar, this is a separate meaning than "this particular instance" (or, more relevant, 'this particular set of instances') has virtual attribute foo actually loaded.

What solution do you think should be pursued? Do we:

a) Filter tilter var x above so it's only writing non-virtual attributes to the cache? b) Make a (separate?) cache on QuickBuilder that only applies to the instances it's making? c) something else?

[elpete]

Good news! I believe this is fixed in the upcoming v6 beta (breaking change is dropping ColdBox 5 support)

[MordantWastrel]

Resolved with v6