Laravel as client for Strava 3rd party API

Question

I’m developing a Laravel app which acts as a client to consume a 3rd party Strava API. My app also functions as an API for a frontend SPA. The user (which is just me for the time being) will already be ‘logged in’ via SPA and Laravel’s Sanctum (cookie-based session auth) so when the user is prompted to log in to their Strava account, this is merely for the purpose of fetching the activities (e.g. run, swim or whatever). Since the activities returned from Strava are immediately saved to the database, a full OAuth flow is not required as I have no need to store the access token and/or deal with refresh tokens.

The Strava activities are fetched and saved by two services:

Strava: gets the auth code, exchanges it for the athlete (user) and gets the activities. This service is bound via a singleton method to a custom provider and called from the controller via a facade

StravaActivity: Maps the returned activities and saves them to the database

What are people’s suggestions for improving this code? Some things I’m unsure of:

• The store action method being called by the handleCallback method in the controller... but from a UX perspective, I want the user to be able to fetch and save the activities via one click from the SPA (albeit having to log in to Strava en route)

• Whether the mapping function should be a class in its own right... though feel this might be over-engineering a small project

• Exceptions: Wanted to throw and catch in the services and simply relay any error to the controller to keep the controller slim

Here’s the relevant code:

Routes:

use App\Http\Controllers\StravaController;
use Illuminate\Support\Facades\Route;
Route::get(
 '/strava/auth',
 [StravaController::class, 'redirectToStrava']
)->name('web.strava.redirectToStrava');
Route::get(
 '/strava/auth/handleCallback',
 [StravaController::class, 'handleCallback']
)->name('web.strava.handleCallback');

StravaController:

namespace App\Http\Controllers;
use App\Facades\Strava;
use App\Services\StravaActivity;
use Illuminate\Http\Request;
use Illuminate\Http\JsonResponse;
use Illuminate\Http\RedirectResponse;
class StravaController extends Controller
{
 private $stravaActivity;
 public function __construct(StravaActivity $stravaActivity)
 {
 $this->stravaActivity = $stravaActivity;
 }
 public function redirectToStrava(): RedirectResponse
 {
 return Strava::getAuthCode();
 }
 public function handleCallback(Request $request): JsonResponse | RedirectResponse
 {
 if (!$request->has('code')) {
 // todo: redirect back to relevant SPA page
 return redirect('/')->withErrors('Auth failed');
 }
 return $this->store($request->code);
 }
 public function store(string $authCode): JsonResponse
 {
 $latestActivities = Strava::getLatestActivities($authCode);
 $response = $this->stravaActivity->saveActivities($latestActivities);
 if ($response['success'] === false) {
 return response()->json(['error' => $response['message']], 422);
 }
 return response()->json($response['recentActivities'], 201);
 }
} 

Strava (service):

namespace App\Services;
use Exception;
use Illuminate\Http\Client\Response;
use Illuminate\Http\RedirectResponse;
use Illuminate\Support\Facades\File;
use Illuminate\Support\Facades\Http;
use Illuminate\Support\Facades\Log;
class Strava
{
 private $stravaOauthUri = 'https://www.strava.com/oauth';
 private $stravaUri = 'https://www.strava.com/api/v3';
 private $stravaClientId;
 private $stravaSecretId;
 private $stravaRedirectUri;
 public function __construct(
 string $stravaClientId,
 string $stravaSecretId,
 string $stravaRedirectUri,
 ) {
 $this->stravaClientId = $stravaClientId;
 $this->stravaSecretId = $stravaSecretId;
 $this->stravaRedirectUri = $stravaRedirectUri;
 }
 public function getAuthCode(
 string $scope = 'read_all,profile:read_all,activity:read_all'
 ): RedirectResponse {
 $query = http_build_query([
 'client_id' => $this->stravaClientId,
 'response_type' => 'code',
 'redirect_uri' => $this->stravaRedirectUri,
 'scope' => $scope,
 'state' => 'strava',
 ]);
 return redirect("{$this->stravaOauthUri}/authorize?{$query}");
 }
 public function getLatestActivities(string $authCode): array
 {
 try {
 $tokenData = $this->getAthleteWithTokens($authCode);
 return $this->getActivities($tokenData['access_token']);
 } catch (Exception $error) {
 return [
 'success' => false,
 'message' => $error->getMessage()
 ];
 }
 }
 private function getAthleteWithTokens(string $authCode): array
 {
 $url = "{$this->stravaOauthUri}/token";
 $config = [
 'client_id' => $this->stravaClientId,
 'client_secret' => $this->stravaSecretId,
 'code' => $authCode,
 'grant_type' => 'authorization_code'
 ];
 $response = Http::post($url, $config);
 if ($response->ok()) {
 return json_decode($response->getBody(), true);
 }
 $this->throwError($response);
 return [];
 }
 private function getActivities(string $token): array
 {
 $url = "{$this->stravaUri}/athlete/activities";
 $response = Http::withToken($token)->get($url);
 if ($response->ok()) {
 return json_decode($response->getBody()->getContents(), true);
 }
 $this->throwError($response);
 return [];
 }
 private function throwError(Response $response): void
 {
 $statusCode = $response->getStatusCode();
 $jsonResponse = $response->json();
 $errorMessage = $jsonResponse['message'] ?? 'Strava Service not available';
 Log::error(
 'Error getting Strava activities',
 [
 'status' => $statusCode,
 'message' => $errorMessage
 ]
 );
 throw new Exception("Strava API error: {$statusCode}: {$errorMessage}");
 }
}

StravaActivity (service):

namespace App\Services;
use App\Models\StravaActivity as StravaActivityModel;
use Illuminate\Database\QueryException;
use Illuminate\Support\Facades\Log;
use Illuminate\Support\Facades\Schema;
class StravaActivity
{
 public function saveActivities(array $activities): array
 {
 $mappedActivities = $this->mapActivities($activities);
 try {
 $recentActivities = $this->storeActivities($mappedActivities);
 } catch (QueryException $error) {
 $returnErrorMessage = $error->getMessage();
 Log::error(
 'Error saving Strava activities',
 [
 'message' => $error->getMessage()
 ]
 );
 $returnErrorMessage = 'SQL error: Strava activity(ies) not persisted';
 return [
 'success' => false,
 'message' => $returnErrorMessage
 ];
 }
 return [
 'success' => true,
 'recentActivities' => $recentActivities
 ];
 }
 private function mapActivities(array $activities): array
 {
 $dataStatisticsToBeMapped = Schema::getColumnListing('strava_activities');
 return array_map(function ($activity) use ($dataStatisticsToBeMapped) {
 $activity['strava_id'] = $activity['id'];
 $activity['map_polyline'] = $activity['map']['summary_polyline'];
 unset($activity['id']);
 unset($activity['map']);
 return array_filter($activity, function ($statItem) use ($dataStatisticsToBeMapped) {
 return (in_array($statItem, $dataStatisticsToBeMapped));
 }, ARRAY_FILTER_USE_KEY);
 }, $activities);
 }
 private function storeActivities(array $mappedActivities): array
 {
 $recentActivities = [];
 foreach ($mappedActivities as $mappedActivity) {
 $newActivity = StravaActivityModel::firstOrCreate(
 ['strava_id' => $mappedActivity['strava_id']],
 $mappedActivity
 );
 if ($newActivity->wasRecentlyCreated) {
 array_push($recentActivities, $mappedActivity);
 }
 }
 return $recentActivities;
 }
}

• Sᴀᴍ Onᴇᴌᴀ: Welcome to Code Review! Is there example code demonstrating instantiation and use of StravaController? If so, please edit to provide this code. In many Laravel projects controllers are connected to web and/or API routes though it is not a requirement.
• ah5: Hi Sam, thanks for your response. Yes, I can see now how it might be a tad confusing without the routes file. I've edited accordingly. The partial OAuth flow is initiated by the redirectToStrava route/method

Answer 1

Some minor points from me, just after a cursory look at the code. My PHP is rusty, but I know unset can destroy more than one variable. Useful to know.

I would try to simplify function saveActivities, and make the control flow more straightforward. This should be equivalent (but untested):

public function saveActivities(array $activities): array
{
 $mappedActivities = $this->mapActivities($activities);
 try {
 $recentActivities = $this->storeActivities($mappedActivities);
 
 return [
 'success' => true,
 'recentActivities' => $recentActivities
 ];
 } catch (QueryException $error) {
 Log::error(
 'Error saving Strava activities',
 [
 'message' => $error->getMessage()
 ]
 );
 return [
 'success' => false,
 'message' => 'SQL error: Strava activity(ies) not persisted';
 ];
 }
}

Changes:

• Don't reassign $returnErrorMessage, just return $error->getMessage(); first, and then the static message. By removing this variable, we shorten the code by 2 lines.
• The return true statement can go up under $recentActivities = $this->storeActivities($mappedActivities);. I believe the two possible outcomes (success/failure) are now better separated, and the risk of a logic error is decreased.

Your exception handling routine does not cover the whole function: $mappedActivities = $this->mapActivities($activities); could still fail. Maybe you could simply move it inside your try block. This further reduces the number of lines of code.

• ah5: Thanks Kate, appreciate your input. All seem like sensible improvements (Heaven knows why I'm reassigning the error message!). And didn't know that about unset. Will investigate

Answer 2

Review

• It is great that type declarations are used for function arguments, return values and class properties.
• There is good usage of the Laravel and Eloquent methods, as well as the Http facade
• Formatting is consistent and code is easy to read

Suggestions

Constructor and properties can be simplified

As of the time of writing PHP 8.3 and 8.4 are the currently supported versions with Active support. With PHP 8 Contructor Promotion can be used to greatly simplify the code in the constructors.

The StravaController class can go from this:

class StravaController extends Controller
{
 private $stravaActivity;
 public function __construct(StravaActivity $stravaActivity)
 {
 $this->stravaActivity = $stravaActivity;
 } 

to this:

class StravaController extends Controller
{
 public function __construct(private StravaActivity $stravaActivity)
 {
 }

And the Strava service:

class Strava
{
 private $stravaOauthUri = 'https://www.strava.com/oauth';
 private $stravaUri = 'https://www.strava.com/api/v3';
 private $stravaClientId;
 private $stravaSecretId;
 private $stravaRedirectUri;
 public function __construct(
 string $stravaClientId,
 string $stravaSecretId,
 string $stravaRedirectUri,
 ) {
 $this->stravaClientId = $stravaClientId;
 $this->stravaSecretId = $stravaSecretId;
 $this->stravaRedirectUri = $stravaRedirectUri;
 }

can be simplified to:

class Strava
{
 private $stravaOauthUri = 'https://www.strava.com/oauth';
 private $stravaUri = 'https://www.strava.com/api/v3';
 public function __construct(
 private string $stravaClientId,
 private string $stravaSecretId,
 private string $stravaRedirectUri,
 ) {
 }

Multiple parameters can be passed to unset()

In StravaActivity::mapActivities() are two calls to unset() (within the callback passed to array_map()):

 unset($activity['id']);
 unset($activity['map']);

unset() can be used to destroy more than one variable in one expression:

 unset($activity['id'], $activity['map']);

Arrow function has simpler format

Also within StravaActivity::mapActivities() is this statement:

 return array_filter($activity, function ($statItem) use ($dataStatisticsToBeMapped) {
 return (in_array($statItem, $dataStatisticsToBeMapped));
 }, ARRAY_FILTER_USE_KEY);

Since there is only one line in the anonynous function, one could use an arrow function instead to avoid having a separate scope. While it may not lead to fewer lines it could be seen as easier to read.

 return array_filter(
 $activity, 
 fn($statItem) => in_array($statItem, $dataStatisticsToBeMapped), 
 ARRAY_FILTER_USE_KEY
 );

• ah5: Thanks Sam, appreciate your review... and for keeping me up to date on my PHP! It's been a while and hadn't realised I could simplify the constructor bloat :) As for arrow function suggestion / keeping logic in scope, that's a good suggestion. It could definitely be more clear.