RobThree / HumanoID

Friendly ID generator. Converts integers to words and back.
MIT License
7 stars 4 forks source link

Feature Request: Laravel (Eloquent Model) Adapter #8

Open caendesilva opened 2 years ago

caendesilva commented 2 years ago

Would be really cool to have a Laravel adapter that provides a trait to integrate the HumanoID to Eloquent Models. I'm thinking of caching the generated ID in the database, but adding a creating method to the boot function of a model could be used to automatically generate and save a HumanoID.

The adapter could also bind the instance as a singleton in the Laravel service container to save CPU costs.

RobThree commented 2 years ago

I'm more of a .Net / C# developer so most of this is Chinese to me 😛 But unless when you have a very good, specific, reason to store the generated HumanoID in the database I would just use the HumanoID representation in the application layer in the user-facing parts of the applications but convert to an int as soon as possible.

I'm not sure what Eloquent Models are and if I'm maybe misunderstanding your request, so please, if I do, let me know.

caendesilva commented 2 years ago

I originally thought that generating the HumanoIDs would take much longer, and thus thought I would need to cache them to save computing time. But when loading the generator as a singleton it takes an average of 0.15~ms per integer-to-humanoid operation which I'm more than happy with.

In essence, Eloquent models are an abstraction for interacting with database rows in an object-oriented manner.

The only concern regarding a possible adapter is how it works with route model binding. In practical terms, if I make an HTTP request to users/humano-id-name, can Laravel find it? It's pretty late here, but I will look into that tomorrow so if possible, keep this issue open until then as I think it could be useful for Laravel devs to know 🙂

caendesilva commented 2 years ago

Okay, I have not tried it with route model binding, but I don't think it's needed as there are other ways.

Here is how I implemented a way to use HumanoIDs as pretty URI slugs for Eloquent models using numerical IDs.

// routes/web.php
Route::get('/users/{user}', function (string $user) {
    return (new UserViewController)->show($user); 
});
class UserViewController extends Controller
{
    public function show(string $user)
    {
        try {
            $id = app('HumanoIDGenerator')->generator->parse($user);
        } catch (\RobThree\HumanoID\Exceptions\LookUpFailureException) {
            abort(404);
        }

        $user = User::findOrFail($id);

        return view('users.show', [
            'user' => $user,
        ]);
    }
}
/**
 * Intermediary class that is bound as a Singleton in the AppServiceProvider.
 */
class HumanoIDGenerator
{
    public \RobThree\HumanoID\HumanoID $generator;

    public function __construct()
    {
        $this->generator = \RobThree\HumanoID\HumanoIDs::spaceIdGenerator();
    }
}
class AppServiceProvider extends ServiceProvider
{
    /**
     * Register any application services.
     *
     * @return void
     */
    public function register()
    {
        $this->app->singleton('HumanoIDGenerator', function ($app) {
            return new HumanoIDGenerator($app->make(HumanoIDGenerator::class));
        });
    }
}

Testing it:

My user ID is 0.

Visiting /users/0 returns a 404, as does /users/foo.

However, visiting /users/andromeda returns my user model!

So considering how easy this was, I don't think a Laravel adapter is actually needed.

RobThree commented 2 years ago

So considering how easy this was, I don't think a Laravel adapter is actually needed.

Ok, but to me the try...catch in every controller method where you'd use a HumanoID seems a bit cumbersome. In .Net I'd use the modelbinding stage or something in the pipeline before the request even reaches the controller and I guess that's what you wanted to achieve in the first place and that does make sense to me. I wouldn't want my controller to be "forced" to take a string as argument either, I'd prefer it take an (int) id too. It just that I'm not familiar enough with Laravel to provide any decent input on this matter.

Maybe a specific Laravel package could be created that takes the HumanoID package as a dependency and that implements all the Laravel specific stuff? Does that make sense to anyone?

I'm sorry if I'm using the incorrect terminology in this matter that might confuse things further.

image

caendesilva commented 2 years ago

Those are great points! I needed the try catch to properly return a 404 response but I do agree it is a bit awkward. My example was a quickly sketched out minimal viable solution.

Maybe a specific Laravel package could be created that takes the HumanoID package as a dependency and that implements all the Laravel specific stuff? Does that make sense to anyone?

This sounds exactly what I was thinking from the beginning! You're right on with the terminology, and I love the dog!

Laravel has what is called implicit route model binding, which basically allows you type hint a model to automatically resolve the model through dependency injection, but I think the native Laravel binding only works with values stored in a database but I am not at all sure. But that is something a Laravel adapter package could handle.

RobThree commented 2 years ago

But that is something a Laravel adapter package could handle.

Soooo...

200w

😁

caendesilva commented 2 years ago

I'll see what I can do! Want to use the package a bit further to fully understand it first though :)

mallardduck commented 2 years ago

Thanks for looking into this stuff @caendesilva - these weren't use cases I was needing immediately but are areas I wanted to explore. As always I see you're hecking fast at looking into interesting areas to play!

To be honest my use case was literally just to add debug meta data to some Blade elements in a personal laravel project. So for me it was always a small use case of rendering ints for about 30 elements on a page at a time. So speed wasn't too important at all.

However with the idea of using it for route binding I do think there could be a use case for a model adapter. Even if you can get by decoding via the generator on the fly, I do think if you want a URL Slug then saving it to the database might be reasonable.

All that said, please let me know if you did want to take on that laravel specific package. I'd love to take that on myself, but if you're already working on it I'll leave you to it!

caendesilva commented 2 years ago

@mallardduck

Thanks for looking into this stuff @caendesilva - these weren't use cases I was needing immediately but are areas I wanted to explore. As always I see you're hecking fast at looking into interesting areas to play!

Thanks haha, I like to think outside of the box 🙂

All that said, please let me know if you did want to take on that laravel specific package. I'd love to take that on myself, but if you're already working on it I'll leave you to it!

I got my hands full with a lot of projects, and I think you can do a much better job than me so I think you should go for it! I'm happy to contribute with PRs if there is anything I can help with. But you take the lead!

caendesilva commented 2 years ago

However with the idea of using it for route binding I do think there could be a use case for a model adapter. Even if you can get by decoding via the generator on the fly, I do think if you want a URL Slug then saving it to the database might be reasonable.

Sharing a snippet that could be useful for the adapter that I made the other day to add UUID's when creating a new model. Could be used to generate the HumanoID if we want to save it to the database.

/**
 * The "booted" method of the model.
 *
 * @return void
 */
protected static function booted()
{
    static::creating(static function (Event $event) {
        if ($event->uuid === null) {
            $event->uuid = Str::uuid();
        }
    });
}

/**
 * Get the route key for the model.
 *
 * @return string
 */
public function getRouteKeyName()
{
    return 'uuid';
}
mallardduck commented 2 years ago

After seeing the benchmarks I do think it'll make more sense to have the laravel integration package save to DB by default. This keeps things much simpler for both the lookup of HumanoID based slugs and the generation of URLs with those slugs. This seems far simpler than the alternative of caching the whole object persistently.

RobThree commented 2 years ago

save to DB by default

Save what to be exact? The HumanoID (string representation of an ID) you mean? Or...?

This seems far simpler than the alternative of caching the whole object persistently.

You mean by "the whole object" a HumanoID generator? Unless you're actually pressed for performance you shouldn't be afraid to just instantiate a generator per request. As long as you're not loading a huge wordset you should be fine.

But maybe I'm interpreting this comment wrong?

caendesilva commented 2 years ago

I think that for the Laravel integration I'm not sure if instantiating the HumanoID generator object is that big of a factor since the instantiation time is much lower than I think any of us expected.

However: the big benefit we would get by saving the HumanoID string representation in the database is that we can natively use the Laravel route model binding.

But performance-wise I'm not actually sure if there will be a big difference for practical use. If you feel like doing it I would love to see the results for a benchmark comparing the two.

I think a bigger factor is the ease of implementation for both methods. When saving to the database we'll need to deal with migrations. Either we have an entirely new table that maps HumanoID strings with database primary keys, or users will need to add a migration column to all the models they want to use HumanoIDs for.

I'm not all opposed to this, but it's a factor to consider. If the speed/performance distance is negligible I think from a user (user as in the developer using the package) perspective, parsing on the fly would be easiest to implement.

For those who need a ton of performance I'd suggest they make a plugin that utilizes Laravel Octane or similar, a submit a PR for that!

mallardduck commented 2 years ago

HumanoID (string representation of an ID)

Correct.

Basically with Laravel we can (I'll do it later) make a package that will provide people with a Trait they can add to their DB models. Then it will automatically generate the HumanoID string from the ID only at initial creation time.

As long as you're not loading a huge wordset you should be fine.

However you raise a good point that the generator shouldn't be too costly per request. So I'll definitely do some benchmarking of that workflow itself. Because right now I was just basing it on numbers from a test I hastily put together but didn't commit.

Basically it just parsed a random HumanoID, then generated multiple to simulate parsing a URL slug to an ID, and then subsequently rendering URLs based on generated values from HumanoID. The results didn't look great at all.

I hadn't pushed that test since I thought of it after the PR was merged, but before I decided to crack a beer and start playing games. 🍻 Ultimately I'll see how the workflows play out in a real Laravel app to see what bottlenecks I run into if any.

caendesilva commented 2 years ago

Is there any way to see how much memory the object takes?

Dan, you mentioned Redis, I was thinking about that as well. I don't have much practical knowledge with it, but it could be cool if the object could be stored in Redis. Though considering I/O throughput and serialization/deserialization I doubt that would be faster.

mallardduck commented 2 years ago

@caendesilva yeah there's a lot of interesting areas we could go with it in the integration package. especially when I consider octane type runtimes - since that could be one "true" (and native) way to have a single instance shared among all request.

One that doesn't necessarily require serialization to redis at all. However for more traditional (php-fpm) based deployments of laravel doing redis+serialization would be much simpler.

Edit: I'll let you gents know what I have the integration package as a proof. If I find myself hitting the Balmer peak here as i game and drink I may be drawn back to my mac for some coding! 🤣

caendesilva commented 2 years ago

@mallardduck There are a lot of interesting aspects here, and a lot I haven't thought about or played around with before.

Enjoy your games and drinks, I'm about to call it a night so no need to rush anything for our sake :)

Looking forward to seeing what you come up with. Cheers!

caendesilva commented 2 years ago

I think a great idea for the Laravel adapter is a facade to easily access the application service container singleton instance as this syntax (which I am noting here because I'm having trouble remembering it) is pretty awkward IMO.

app('HumanoIDGenerator')->generator->parse($value)
mallardduck commented 2 years ago

The ideal syntax for the integration package won't include the ->generator part at all. That's just what you'll get by default IMO.

mallardduck commented 2 years ago

Further discussions can now take place on the package that's WIP here: https://github.com/mallardduck/laravel-HumanoID

mallardduck commented 2 years ago

A specific issue to carry over the topic into: https://github.com/mallardduck/laravel-HumanoID/issues/2

caendesilva commented 2 years ago

Wooo 🎉 great job Dan! Should we close this issue?

mallardduck commented 2 years ago

Yeah, i think we can close this one out. Unless we want to keep it open until I tag a stable release?