Fixed issue where virtualAttributes were not being applied to discriminated child entities when loading through the parent

[ryanalbrecht]

If you add a virtual attribute to a parent entity and load a discriminated child entity through that parent entity. The virtual attributes will not be applied to the child entity as quick will load a new instance of the child class and then use hydrate() to populate the entity. I fixed this by checking if the parent entity has any virtual attributes and then manually calling appendVirtualAttribute on the child class.

Additionally, thinking in terms of performance I cached the key name of all virtual entities in a new property called _virtualAttributes. This would eliminate the overhead (albeit a tiny one) of looping through all the attributes in the parent entity to check if it is a virtual attribute.

[homestar9]

Nicely done. I can't help but wonder if similar logic could be applied to the issue where discriminated entities lose their casts that were defined on the parent. https://github.com/coldbox-modules/quick/issues/233

[ryanalbrecht]

@homestar9 fixed that one up aswell.

I have removed the call to hydrate and manually assigned the attribute data with assignAttributesData() as this function is what performs the cast on the properties. I dont really forsee any problems with this as is essentially the same as hydrate.

Also @elpete I removed the below code from QuckBuilder.cfc as it seemed a bit pointless to me. Could you expand on what this doing and if it is needed?

getEntity()
    .keyNames()
    .each( function( key, i ) {
        data[ childClass.keyNames()[ i ] ] = data[ key ];
    } );

[homestar9]

You're a rock star! This is awesome. I will download your fork and test these changes out.

[ryanalbrecht]

You're a rock star! This is awesome. I will download your fork and test these changes out.

sweet, let me know if you run into any issues

[homestar9]

@ryanalbrecht It looks like the casts issue is still present, unfortunately. For some reason, populateAttributes() is ignoring the casted values.

Edit: I made a mistake. The casts issue I am referring to isn't #233. Instead, it has to do with a regression from version 4 where casts aren't preserved after persistence and/or when getting mementos.
This regression still exists: https://github.com/coldbox-modules/quick/issues/203

Your branch fixes #233, so thank you very much!

[ryanalbrecht]

@homestar9 Ok I think I caught that one aswell.

The problem was that when you call save() on an entity quick will reset the _castCache property with the value retrieved from castValueForSetter() (the getter value of the cast) method. Quick 4.1.2 would recast the attributes after saving by calling populateAttributes(), but with later versions of quick, the castValueForGetter() (the setter value of the cast) method would check if the attribute exists in _castCache and return it if it did. I added a new parameter to specifiy if you would like to force a cast when calling castValueForGetter() and then manually set the attribute values after the entity has been written to the database.

[homestar9]

This is awesome! Thank you for doing that. I pulled the latest commit and I get an exception in my integration tests with Quick's JsonCast.cfc. I'll see if I can nail down what might be happening and respond to this thread.

[ryanalbrecht]

This is awesome! Thank you for doing that. I pulled the latest commit and I get an exception in my integration tests with Quick's JsonCast.cfc. I'll see if I can nail down what might be happening and respond to this thread.

Aarrrgg. I thought I had it. Could you paste a copy of the exception and I could potentially solve the issue?

[homestar9]

I think there's a bug in your JsonCast.cfc: https://github.com/ryanalbrecht/quick/blob/399bbb03c6ca6a8b56bf9827979380ceaed77935/models/Casts/JsonCast.cfc#L18C3-L20C4

Should be:

if( isStruct( arguments.value ) || isArray( arguments.value ) ){
    return arguments.value
}

Valid JSON can be {} or [].

[ryanalbrecht]

Ok I pushed up a fix. @elpete probably needs to weigh in on this change though. This is a fairly significate departure from how a person would expect the JsonCast 'caster' to work. This check was probably intentionally excluded in the original code as an exception would be raised if invalid json was loaded from the database, with this change any value could be saved and loaded to the database and the caster would silently fail.

I need to chew on this for a bit

[homestar9]

I see what you mean, and I agree the most recent change is a bit of a departure.

I have been thinking about the JsonCast.cfc for the last hour or so and, in my opinion, I like the following approach best:

public any function get(
    required any entity,
    required string key,
    any value
) {

    // if the value is already a struct or an array, let it pass through
    if ( isStruct( arguments.value ) || isArray( arguments.value ) {
        return arguments.value;
    }

    if ( isNull( arguments.value ) ) {
        return javacast( "null", "" );
    }

    if ( arguments.value == "" ) {
        return "";
    }

    return deserializeJSON( arguments.value );
}

I believe @elpete's original intention of throwing an exception if arguments.value is an unexpected type is preserved with the above code. However, I changed it to allow a struct or an array to pass through as-is, just as if it was already cast from the original JSON object.

[ryanalbrecht]

Hi Dave,

Although highly unlikely, JSON can accept primitive types as valid json, so you should not rely on accepting only accepting structs and arrays. All the below is all valid json

[ "asdasd", 123 ] { "prop": "value" } "some random text" 123 false

[homestar9]

Darn, that's right. What about changing this line:

// if the value is already a struct or an array, let it pass through
if ( isStruct( arguments.value ) || isArray( arguments.value ) {
    return arguments.value;
}      

To this:

// if the value is already deserialized, return it
if ( 
    isSimpleValue( arguments.value ) || 
    isStruct( arguments.value ) || 
    isArray( arguments.value ) 
){
    return arguments.value;
}

[homestar9]

@elpete just wanted to check in to make sure you saw this PR. :)

[bdw429s]

This pull request has been mentioned on Ortus Solutions Community. There might be relevant details there:

https://community.ortussolutions.com/t/using-discriminated-entities-when-the-base-class-has-no-knowledge-of-child-classes/9728/8