Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add memoization for build referencable schemas #30

Merged

Conversation

jpedryc
Copy link

@jpedryc jpedryc commented Dec 6, 2024

TLDR

Memoization for referencable schemas to limit required instance builds.

Important

Related to discussion #29.


User benefit

Although the majority of declared schema properties are trivial:

public function build(): SchemaContract
{
    return Schema::object('ExampleObject')
        ->properties(
             Schema::string('example-string-prop')->nullable(),
             Schema::integer('example-id')->example(123),
             // ...
        )
}

sometimes more resource demanding actions are being taken, e.g. model-querying to get example values:

Schema::integer('status_id')
    ->enum(Status::pluck('id'))

Note

Another examples are:

  • querying schema descriptions
  • creating custom Collections for ...->enum(...)s
  • accessing some other third-party services to achieve e.g. one of the above

Now, assuming we have a bit more popular Schema that is loaded in multiple other Schemas, e.g. a UserSchema, that is being also referenced with the same objectId (UserSchema::ref('user')):

// Parent A
class ParentASchema extends SchemaFactory implements Reusable
{
    public function build(): SchemaContract
    {
        return Schema::object('ParentA')
            ->properties(
                // ...
                UserSchema::ref('user'),
            );
    }
}

// Parent B
class ParentBSchema extends SchemaFactory implements Reusable
{
    public function build(): SchemaContract
    {
        return Schema::object('ParentB')
            ->properties(
                // ...
                UserSchema::ref('user'),
            );
    }
}

⚠️ The schema-component will be build every time per reference ⚠️

Implementation Overview

diff --git a/src/Concerns/Referencable.php b/src/Concerns/Referencable.php
index fd36525..4dfb8fe 100644
--- a/src/Concerns/Referencable.php
+++ b/src/Concerns/Referencable.php
@@ -14,6 +14,8 @@ use Vyuldashev\LaravelOpenApi\Factories\SecuritySchemeFactory;
 
 trait Referencable
 {
+    static $buildReferencableSchemas = [];
+
     public static function ref(?string $objectId = null): Schema
     {
         $instance = app(static::class);
@@ -38,6 +40,6 @@ trait Referencable
             $baseRef = '#/components/securitySchemes/';
         }
 
-        return Schema::ref($baseRef.$instance->build()->objectId, $objectId);
+        return self::$buildReferencableSchemas[static::class . "_{$objectId}"] ??= Schema::ref($baseRef.$instance->build()->objectId, $objectId);
     }
 }

To make sure we're not re-building schema components of the same class and with the same object ID - we make use of the Memoization Technique.

Test Overview

class MemoizationTest extends TestCase
{
    public function testReferencableInstanceBuildOnce(): void
    {
        self::assertEquals($e1ObjectId = (new ExampleSchema)::ref('e1'), (new ExampleSchema)::ref('e1'));
        self::assertNotEquals($e1ObjectId, (new ExampleSchema)::ref('e2'));
    }
}

We test if objects of the same class and object ID are truly equal. Additionally - we make sure that the object ID matters in the second assertion.

Schema components of the same class and object ID won't be build every
time from scratch which improves performance.
@jpedryc jpedryc force-pushed the add-ref-schemas-memoization branch from 5612b52 to 0fafbfb Compare December 6, 2024 02:13
@jpedryc
Copy link
Author

jpedryc commented Dec 6, 2024

I've run manual Clockwork tests with these changes on one of my internal projects (just as an addendum) - checked package-provided /openapi endpoint:

Current main With memoization added
Resp. time ~1080 ms ~600 ms
Mem. usage ~10 MB ~8 MB
# Models loaded 6599 2242
# DB queries 188 47
DB time 46 ms 14 ms

@TartanLeGrand
Copy link
Owner

LGTM!

@TartanLeGrand TartanLeGrand merged commit 7657892 into TartanLeGrand:main Dec 9, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants