Fix: Accessor caching

[CamKem]

As mention in the Telegram group, whilst writing a new parser for link previews, I've discovered we are calling $question->content & $question->answer around 8 times per post, resulting in needing to run all of the parsers for these model attributes each time we call them.

This PR introduces a caching technique using the new Context facade, allowing us to cache the values during the current request. As the cache needs to be busted when we have dirty values, to ensure that the hashtags are correctly synced when a user is editing a post, I have ensured this happens.

EDIT:
PR Updated to use a static property on the ParsableContent service as the in-memory static cache. Cache key is removed when the attribute is dirty & flushed when the app is terminated.

This will reduce it to only 1 parse for each of the 2 attributes, and will drastically improve our load times on the Pinkary feed & help as the site grows to introduce more content parsing.

I have added Tests for the new logic to ensure full coverage.

[steven-fox]

Can someone help me understand why this implementation strategy is needed/better than using standard Eloquent Accessor caching?

[CamKem]

Can someone help me understand why this implementation strategy is needed/better than using standard Eloquent Accessor caching?

Mainly for your work on Hashtags, but more generally for any logic that requires reading actual new data from a dirty state in the same request. In this instance, the QuestionObserver update method is passed the model with cached values, rather than the updated value (Attribute accessors don't cache bust) & dirty state triggers the Hashtag parser is parsing the old value, so we needed a way to bust the cache within the same request when the value is dirty & the property is called.

Obviously I wanted to understand what was going on, so I looked and found that the framework is just not designed to facilitate anything outside of returning a single Attribute::make() call within the accessor method, so conditionals on dirty or something like that is out of the question. HasAtrributes trait uses reflection to determine if it should be cached, so it's not designed to handle it & actually triggers infinite recursion on a conditional and kills the application.

There might be an opportunity for a framework PR looking into the option to cache bust if the value is dirty, but for now this is our best solution.

Interestingly too, the defining accessors using the Attribute class adds a bit of overhead, which on benchmarking doesn't give us any performance gain over the current repeated parsing, obviously that would change as we add additional parser, but if we use the old syntax the benchmarks show an improvement.

[steven-fox]

Can someone help me understand why this implementation strategy is needed/better than using standard Eloquent Accessor caching?

Mainly for your work on Hashtags, but more generally for any logic that requires reading actual new data from a dirty state in the same request. In this instance, the QuestionObserver update method is passed the model with cached values, rather than the updated value (Attribute accessors don't cache bust) & dirty state triggers the Hashtag parser is parsing the old value, so we needed a way to bust the cache within the same request when the value is dirty & the property is called.

Obviously I wanted to understand what was going on, so I looked and found that the framework is just not designed to facilitate anything outside of returning a single Attribute::make() call within the accessor method, so conditionals on dirty or something like that is out of the question. HasAtrributes trait uses reflection to determine if it should be cached, so it's not designed to handle it & actually triggers infinite recursion on a conditional and kills the application.

There might be an opportunity for a framework PR looking into the option to cache bust if the value is dirty, but for now this is our best solution.

Interestingly too, the defining accessors using the Attribute class adds a bit of overhead, which on benchmarking doesn't give us any performance gain over the current repeated parsing, obviously that would change as we add additional parser, but if we use the old syntax the benchmarks show an improvement.

Thanks for the answer Cam. Very interesting. Does sound like a potential PR for the framework. I might work on that if I have time.

Out of curiosity, I tried a hack, and it appears to work lol. Obviously we still have the performance concern as you stated but a funny hack nonetheless.

    protected function answer(): Attribute
    {
        return Attribute::make(
            get: fn (?string $rawAnswer): ?string => $rawAnswer ? (new ParsableContent())->parse($rawAnswer) : null,
            set: function (?string $newAnswer): ?string {
                unset($this->attributeCastCache['answer']); // 👀

                return $newAnswer;
            },
        )->shouldCache();
    }

    $question = \App\Models\Question::factory()->create([
        'answer' => 'Original answer @foo',
    ]);

    $originalParsedAnswer = $question->answer; // parsed with @foo link

    $question->answer = 'This is some new content #hashtag';

    $newParsedAnswer = $question->answer; // parsed with #hashtag link