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

Tip 3: Treat pivot models as their own resource #3

Merged
merged 2 commits into from
Aug 2, 2017
Merged

Conversation

adamwathan
Copy link
Owner

@adamwathan adamwathan commented Aug 2, 2017

The next custom actions I'd like to look at are PodcastsController@subscribe and PodcastsController@unsubscribe.

Here are the endpoints:

Route::post('/podcasts/{id}/subscribe',   'PodcastsController@subscribe');
Route::post('/podcasts/{id}/unsubscribe', 'PodcastsController@unsubscribe');

Modelling these endpoints as standard REST actions is a little trickier than our previous examples, because how do we translate subscribe to something like store, update, or destroy?

Let's try modelling it as a store action first. The trick is to ask yourself this question:

"After subscribing, what do I have now that I didn't have before?"

If you can answer that question reasonably, it's probably possible to model the custom action you're refactoring as a store action.

In our case, after subscribing to a podcast, we now have a subscription, which is something we didn't have before. Let's create a new controller!

✅ Create a dedicated SubscriptionsController

If we are creating a new subscription, the standard endpoint structure would be something like this:

- Route::post('/podcasts/{id}/subscribe', 'PodcastsController@subscribe');
+ Route::post('/subscriptions',           'SubscriptionsController@store');

Something worth noticing here is that previously our endpoint had a route parameter, the {id} of the podcast we were subscribing to. Since our new endpoint doesn't have that parameter, we'll have to pass it through in the request body.

Here's what our new controller action would look like now:

class SubscriptionsController
{
    public function store()
    {
        $podcast = Podcast::published()->findOrFail(request('podcast_id'));

        $podcast->users()->attach(Auth::user());

        return response('', 204);
    }
}

Modelling unsubscribe

Let's tackle unsubscribe next. If subscribe becomes creating a subscription, then maybe unsubscribe should become destroying a subscription!

The standard endpoint structure for destroying a resource would look something like this:

- Route::post('/podcasts/{id}/unsubscribe', 'PodcastsController@subscribe');
+ Route::delete('/subscriptions/{id}',      'SubscriptionsController@destroy');

But wait a minute: notice how our /subscriptions endpoint there has an {id} parameter? Subscriptions aren't real models in our system; we don't have the concept of an id for a particular subscription.

How do we delete a subscription by id if subscriptions don't have IDs?

Uncovering a new model

If you look at our implementation, you'll see that subscribing to a podcast creates a new record in our podcast_user pivot table:

$podcast->users()->attach(Auth::user());

If you think about it, doesn't each one of those records represent a user's subscription to a podcast? Each one of those records has an ID, but how can we get it?

Well if we rename podcast_user to subscriptions, we can also create an Eloquent model for working with that table directly called Subscription. Since this table has foreign keys back to users and podcasts, we could even define those as belongsTo relationships on the new model:

class Subscription extends Model
{
    protected $guarded = [];

    public function user()
    {
        return $this->belongsTo(User::class);
    }

    public function podcast()
    {
        return $this->belongsTo(Podcast::class);
    }
}

Remember the line in our controller where we added a pivot record through the attach method? What if we just explicitly create a new subscription instead?

class SubscriptionsController extends Controller
{
    public function store()
    {
        $podcast = Podcast::published()->findOrFail(request('podcast_id'));

        $subscription = Subscription::create([
            'user_id' => Auth::user()->id,
            'podcast_id' => $podcast->id,
        ]);

        return $subscription;
    }
}

Now that we are able to expose a Subscription as it's own resource, it's not so far fetched to expect someone to have the id of the subscription they want to destroy when unsubscribing.

Here's what the controller action could look like:

public function destroy($id)
{
    $subscription = Auth::user()->subscriptions()->findOrFail($id);

    $subscription->delete();

    return response('', 204);
}

Here's what we're left with after this refactoring:

  • PodcastsController, with 7 standard actions and 2 custom actions
  • EpisodesController, with 4 standard actions and no custom actions
  • PodcastEpisodesController, with 3 standard actions and no custom actions
  • PodcastCoverImageController, with 1 standard action and no custom actions
  • SubscriptionsController, with 2 standard actions and no custom actions

Next up: Think of different states as different resources

@uxweb
Copy link

uxweb commented Aug 3, 2017

@adamwathan

Hi Adam,

I ended up with a question after reading this tip. It is about the subscriptions endpoint. If the application has subscriptions for another concept, say news, now we have 2 things users can subscribe.

The first thing that comes to my mind is to nest the subscriptions resource under its own parent resource:

/podcasts/{id}/subscriptions

/news/subscriptions

At first it does look fine. No need to pass the podcast in the request body but this is a nested resource and URL is a little longer.

I think another approach could be to create a single resource that represents the actual concept without be a nested resource:

/podcast-subscriptions
// PodcastSubscriptionsController@store

/news-subscriptions
// NewsSubscriptiosController@store

This second option feels better to me as it is simpler and more expressive than the first one.

What do you think about both options?
Which one feels better for you and why?

Thanks Adam!

@zippex
Copy link

zippex commented Aug 4, 2017

Hi @adamwathan ,

I'm just wondering wheather calling delete() on the subscription model would trigger the default mysql delete cascade from the podcasts_users pivot table? So I have to ensure that there is no cascading within the migration to avoid this pitfall, right?

Regards, s~

@uxweb
Copy link

uxweb commented Aug 4, 2017

@zippex

I think it will do it only if it was defined in the migration.

@lkmadushan
Copy link

@uxweb Why not go with polymorphic relation for subscription. then you can pass a subscription type and keep endpoint as same.

@adamwathan
Copy link
Owner Author

Hey @uxweb, I like this approach:

/podcasts/{id}/subscriptions

You could stick with that approach even if there was only one subscription type if you wanted, nothing wrong with that.

If the idea of "news subscriptions" had no ID associated with it, then I'd probably just do:

/news-subscriptions

Cheers!

@adamwathan
Copy link
Owner Author

@zippex You shouldn't have any problems with cascading deletes unless you set up your migrations that way specifically, but even then I think cascading deletes usually work in the other direction, ie. deleting a user would delete all of that user's subscriptions.

@dailos
Copy link

dailos commented Aug 30, 2017

Hi @adamwathan ,

We love your tip of using a dedicated resource for managing pivot tables.

Sometimes you want to do a bulk action: assign an array of ids instead of one id (i.e, postcast_id is an array of postcast_ids you want to assign or remove to that user , and the amount of ids in the array is quite big).

What do you suggest in this case?

@biblicalph
Copy link

@dailos I think the post endpoint can pass along an array of podcast_ids instead of a single id. This will still look quite clean and allow the use of dedicated pivot model and controller. @adamwathan What do you think of this though?

@dailos
Copy link

dailos commented Sep 10, 2017

@biblicalph we got the same conclusion: an array of ids. In the other hand, sending a new request per id looks cleaner but if the amount of ids is big enough it will be a waste of resources.

@adamwathan
Copy link
Owner Author

@dailos @biblicalph

I think I would try to figure out a way to move bulk updates to it's own controller, maybe by exposing a PATCH endpoint on the whole resource:

Route::patch('subscriptions', 'BulkSubscriptions@update')

Otherwise it sounds like you'd have to do a bunch of messy conditional logic in an existing action.

@ziming
Copy link

ziming commented Oct 21, 2017

Hi @adamwathan

Since BulkSubscriptionsController only have 1 method would a single action controller be better?

class BulkSubscriptionsController extends Controller
{

    public function __invoke(Request $request)
    {
        // bulk sync logic here
    }
}

@adamwathan
Copy link
Owner Author

Hey @ziming! I still like using resourceful action names even if a controller only needs one action. I find if I don't impose that constraint, it gets too easy to start making single action controllers for every tricky action (like SubscribeToPodcastController) and I lose the benefits of uncovering new concepts in my domain like Subscriptions.

@marcelo-kazalukian
Copy link

Hello @adamwathan

Beautiful tips, thanks for everything.

In this line of the destroy method

$subscription = Auth::user()->subscriptions()->findOrFail($id);

Can I use this?

$subscription = Suscription::findOrFail($id);

Cheers.

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.

None yet

8 participants