dblock / strava-ruby-client

A complete Ruby client for the Strava API v3.
https://code.dblock.org/2018/11/27/writing-a-new-strava-api-ruby-client.html
MIT License
97 stars 22 forks source link

Attempting to serialize an activity with `to_json` is broken (Ruby 3.1.2.p20) #72

Closed aroth closed 1 year ago

aroth commented 1 year ago

Great library, thanks for developing and maintaining it!

I've discovered an issue when calling to_json on an activity due to activesupport -> json.rb breaking on the http_response object. I realize this isn't in your lib directly, but wanted to point it out as it'd be nice to be able to serialize an activity without a workaround.

strava-ruby-client: 1.0.0 Ruby: 3.1.2.p20

activity.to_json

../3.1.0/gems/activesupport-7.0.3.1/lib/active_support/core_ext/object/json.rb:159:in `initialize_dup': stack level too deep (SystemStackError)

The object causing the issue is http_response.

Work around:

activity.delete('http_response')
activity.to_json # => { .... }
simonneutert commented 1 year ago

hi @aroth, I am the co-maintainer of this library.

If you would be so kind and try this commit/branch and provide some feedback, whether this would fix the issue.

You should be able to load the gem with my changes, after changing the line of the gem in your Gemfile to:
gem 'strava-ruby-client', github: 'simonneutert/strava-ruby-client', branch: 'serialization-stack-too-deep'

simonneutert commented 1 year ago

i setup a tiny Roda REST-API app and queried an endpoint with a json serialized activity and it worked 🎉 let's hope Rails will be happy with the fix in place, too 🤞

config.ru

# config.ru
require 'bundler/setup'
require 'roda'
require 'strava-ruby-client'

class App < Roda
  plugin :json

  route do |r|
    r.root do
      # GET YOUR TOKEN:
      # https://github.com/simonneutert/strava-ruby-client/tree/serialization-stack-too-deep#command-line-oauth-workflow
      #
      client = Strava::Api::Client.new(
        client_id: '<your-client-id>',
        client_secret: '<your-client-secret>',
        access_token: '<a-valid-generated-token>'
      )
      activities = client.athlete_activities
      activity = activities.first
      { activity: activity } # curl/httpie localhost:9292 🚀
    end
  end
end

run App.app

Gemfile

# Gemfile
# frozen_string_literal: true

source "https://rubygems.org"

gem "rackup", "~> 2.1"
gem "roda", "~> 3.64"
gem 'strava-ruby-client', github: 'simonneutert/strava-ruby-client', branch: 'serialization-stack-too-deep'
aroth commented 1 year ago

@simonneutert Worked for me. Just note the typo:

if options[:skip_http_reponse] || options['skip_http_reponse']

reponse => response

simonneutert commented 1 year ago

After playing around a little last evening, let me share my thoughts with you.

I assume the serializing in Rails is broken, because we returned the class name.

[1] pry(#<RSpec::ExampleGroups::StravaApiClientActivity_2::SerializationToJSON::WhenActivityIsSerializedToJsonWithDefaultEnvironment>)> activity.http_response.to_json
=> "\"#<Strava::Web::ApiResponse:0x0000000109e340e0>\""

This might cause Rails trying to resolve it, going down a rabbit hole without an end.


Here's the thing:

Running

activity = client.activity(1982980795)
a = JSON.parse(activity.to_json)
a.keys.sort

returns

["achievement_count",
 "athlete",
 "athlete_count",
 "available_zones",
 "average_cadence",
 "average_heartrate",
 "average_speed",
 "average_temp",
 "average_watts",
 "best_efforts",
 "calories",
 "comment_count",
 "commute",
 "description",
 "device_name",
 "device_watts",
 "display_hide_heartrate_option",
 "distance",
 "elapsed_time",
 "elev_high",
 "elev_low",
 "embed_token",
 "end_latlng",
 "external_id",
 "flagged",
 "from_accepted_tag",
 "gear",
 "gear_id",
 "has_heartrate",
 "has_kudoed",
 "heartrate_opt_out",
 "highlighted_kudosers",
 "id",
 "kilojoules",
 "kudos_count",
 "laps",
 "leaderboard_opt_out",
 "location_city",
 "location_country",
 "location_state",
 "manual",
 "map",
 "max_heartrate",
 "max_speed",
 "max_watts",
 "moving_time",
 "name",
 "photo_count",
 "photos",
 "pr_count",
 "private",
 "resource_state",
 "segment_efforts",
 "segment_leaderboard_opt_out",
 "similar_activities",
 "splits_metric",
 "splits_standard",
 "sport_type",
 "start_date",
 "start_date_local",
 "start_latitude",
 "start_latlng",
 "start_longitude",
 "suffer_score",
 "timezone",
 "total_elevation_gain",
 "total_photo_count",
 "trainer",
 "upload_id",
 "utc_offset",
 "visibility",
 "weighted_average_watts",
 "workout_type"]

All methods of an Activity are missing when being serialized to JSON.

So, by changing the http_response from a property to a method here in this mixin, we will get back the serialization behaviour we always had and can keep the ratelimit, if I am not mistaken 🤞

I need to work out the details and specs for you later.

dblock commented 1 year ago

I wouldn't make any assumptions about Rails. You can always build a small integration test that combines a Gemfile that has Rails, and an RSpec test that reproduces the issue, ala https://github.com/ruby-grape/grape/blob/45de4884b8cad82dfa3bf2167659742dbb6056d5/gemfiles/multi_json.gemfile#L7 + https://github.com/ruby-grape/grape/tree/master/spec/integration/multi_json.