API should always return a valid JSON body

[BanzaiMan]

Problem

When a user asks for JSON data for a nonexistent gem, currently API returns a plain string.

Steps to reproduce

curl -v https://rubygems.org/api/v1/gems/nonexistentgem.json

Desired Behavior

API always returns valid JSON data

Actual Behavior

$ curl -v https://rubygems.org/api/v1/gems/nonexistentgem.json
*   Trying 151.101.194.2...
* TCP_NODELAY set
* Connected to rubygems.org (151.101.194.2) port 443 (#0)
* TLS 1.2 connection using TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
* Server certificate: f2.shared.global.fastly.net
* Server certificate: GlobalSign CloudSSL CA - SHA256 - G3
* Server certificate: GlobalSign Root CA
> GET /api/v1/gems/nonexistentgem.json HTTP/1.1
> Host: rubygems.org
> User-Agent: curl/7.54.0
> Accept: */*
>
< HTTP/1.1 404 Not Found
< Content-Type: application/json; charset=utf-8
< X-Frame-Options: SAMEORIGIN
< X-XSS-Protection: 1; mode=block
< X-Content-Type-Options: nosniff
< Content-Security-Policy: default-src 'self'; script-src 'self' https://secure.gaug.es; style-src 'self' https://fonts.googleapis.com; img-src 'self' https://secure.gaug.es https://gravatar.com https://secure.gravatar.com; font-src 'self' https://fonts.gstatic.com; connect-src https://s3-us-west-2.amazonaws.com/rubygems-dumps/; frame-src https://ghbtns.com
< Cache-Control: no-cache
< X-Request-Id: 856cb4c3-74d0-432d-be69-419e567fa78e
< X-Runtime: 0.003746
< Strict-Transport-Security: max-age=0
< Content-Length: 32
< Accept-Ranges: bytes
< Date: Wed, 18 Oct 2017 06:17:33 GMT
< Via: 1.1 varnish
< Age: 0
< Connection: keep-alive
< X-Served-By: cache-iad2120-IAD
< X-Cache: MISS
< X-Cache-Hits: 0
< X-Timer: S1508307453.347657,VS0,VE83
< Vary: Accept-Encoding
< Server: RubyGems.org
<
* Connection #0 to host rubygems.org left intact
This rubygem could not be found.

Discussion

As you see, the server returns a plain string This rubygem could not be found. along with the header Content-Type: application/json; charset=utf-8.

This is wrong.

It should either return an empty JSON object {}, or some explanation {"error": "This rubygem could not be found."}.

[colby-swandale]

I agree, we should be returning errors as a JSON body. Though I'm curious if we're breaking backwards compatibility by doing so.

[dwradcliffe]

cc: @bf4 since you talked about this recently

[simi]

What about to introduce this as v2 or with additional parameter for now in v1?

[bf4]

@dwradcliffe I started looking up relevant places in the codebase to consider, but didn't act on it

places to look at found via
https://github.com/rubygems/rubygems.org/search?p=6&q=error&type=&utf8=%E2%9C%93

https://github.com/rubygems/rubygems.org/blob/cb09831cd8e827a821b8cdc203fecd8bbab6b722/config/initializers/honeybadger.rb
https://github.com/rubygems/rubygems.org/blob/f9f6c659d22948af77f1e43eca81b5ca3dec4eeb/test/functional/subscriptions_controller_test.rb
https://github.com/rubygems/rubygems.org/blob/5e430f65900d633de45bc44757214b9ef8f2da33/app/models/concerns/rubygem_searchable.rb
https://github.com/rubygems/rubygems.org/blob/5e430f65900d633de45bc44757214b9ef8f2da33/app/controllers/api/v1/dependencies_controller.rb
https://github.com/rubygems/rubygems.org/blob/5e430f65900d633de45bc44757214b9ef8f2da33/app/controllers/application_controller.rb
https://github.com/rubygems/rubygems.org/blob/5e430f65900d633de45bc44757214b9ef8f2da33/test/unit/web_hook_test.rb
https://github.com/rubygems/rubygems.org/blob/5e430f65900d633de45bc44757214b9ef8f2da33/app/controllers/api/v1/rubygems_controller.rb
https://github.com/rubygems/rubygems.org/blob/f9f6c659d22948af77f1e43eca81b5ca3dec4eeb/test/functional/api/v1/owners_controller_test.rb

I was looking for

• configuration of exceptions app
• any rescue_from (rescuable class level)
• any exception swallowing
• how changing the exceptions app to handle rendering JSON would affect existing tests
• how existing tests are written
• if changes should be made locally or globally

I've written my own public exceptions app to handle 'api' exceptions and have written tests against it

# translating from my RSpec code

class ExceptionsAppTest < ActionDispatch::IntegrationTest

    setup do
      Rails.application.env_config['consider_all_requests_local'] = false
      Rails.application.env_config['action_dispatch.show_exceptions'] = true
   end
     
    teardown do
      Rails.application.env_config['consider_all_requests_local'] = Rails.application.config.consider_all_requests_local
      Rails.application.env_config['action_dispatch.show_exceptions'] = Rails.application.config.action_dispatch.show_exceptions
   end

    test "server returns JSON error for failed server error on JSON request" do
       headers = {
            'Accept' = 'application/json',
           'HTTP_HOST' => 'api.example.com',
            'connection' => 'close'
       }
       get '/i_dont_exist', {}, headers
       assert_response :not_found

       exception_name = "ActionController::RoutingError"
       exception_message = "No route matches [GET] \"/i_dont_exist\""
       expected_body = {name: exception_name, message: exception_message}.to_json
       assert_equal expected_body, response.body
       assert_equal 'application/json; charset=utf-8', response.headers['Content-Type']
    end
end

and see

• https://github.com/rails/rails/blob/5-0-stable/actionpack/lib/action_dispatch/middleware/show_exceptions.rb#L49
• https://github.com/rails/rails/blob/5-0-stable/actionpack/lib/action_dispatch/middleware/public_exceptions.rb#L22-L33

[bf4]

Looks like in the code this is explicit behavior

# "app/controllers/api/v1/rubygems_controller.rb" 
class Api::V1::RubygemsController < Api::BaseController

  def show
    if @rubygem.hosted? && @rubygem.public_versions.indexed.count.nonzero?
      respond_to do |format|
        format.json { render json: @rubygem }
        format.yaml { render yaml: @rubygem }
      end
    else
      render plain: t(:this_rubygem_could_not_be_found), status: :not_found
    end
  end

  def create
    gemcutter = Pusher.new(
      current_user,
      request.body,
      request.protocol.delete("://"),
      request.host_with_port
    )
    gemcutter.process
    render plain: gemcutter.message, status: gemcutter.code
  rescue => e
    Honeybadger.notify(e)
    render plain: "Server error. Please try again.", status: 500
  end

# "test/integration/api/v2/version_information_test.rb"

  test "gem does not exist" do
    request_endpoint(Rubygem.new(name: "nonexistent_gem"), '2.0.0')
    assert_response :not_found
    assert_equal "This gem could not be found", @response.body
    assert_equal "json", response.
  end

I could add this pretty easily

[BanzaiMan]

rubygems/rubygems#2237 reminded me that returning {} for nonexistent gems is probably wrong, since it cannot be distinguished from existent gems which do not have valid versions.

[indirect]

Maybe in the case of a gem that does not exist we could return an empty 404 for format json? But I think I prefer the {"error": "Gem does not exist"} option because it's more human-readable. ¯\_(ツ)_/¯

[sonalkr132]

This is happening because we are using format.any when dealing with 404, and yes this is explicit behaviour.

format.any do
    render plain: t(:this_rubygem_could_not_be_found), status: :not_found
end

I wouldn't suggest changing response text in v1, and porting everything to v2 just for this seems unnecessary to me. api/v1/gems isn't the only endpoint where we are depending on find_rubygem. In my opinion, we should change Content-Type header to text/plain. Client did say Accept: */* and format seems non binding to me for a client side error (couldn't find any doc about server's responsibility on 4xx).

[simi]

🤔 Trying again original curl command today, there is proper (text) content-type sent back. It is not ideal, but it seems better. IMHO it makes sense in terms of compatibility to keep it as is. API client can check for HTTP return code to find out what's responded and also Content-Type to parse the body if needed.

$ curl -v https://rubygems.org/api/v1/gems/nonexistentgem.json
*   Trying 151.101.65.227:443...
* Connected to rubygems.org (151.101.65.227) port 443 (#0)
* ALPN: offers h2,http/1.1
* TLSv1.3 (OUT), TLS handshake, Client hello (1):
*  CAfile: /etc/pki/tls/certs/ca-bundle.crt
*  CApath: none
* TLSv1.3 (IN), TLS handshake, Server hello (2):
* TLSv1.3 (IN), TLS handshake, Encrypted Extensions (8):
* TLSv1.3 (IN), TLS handshake, Certificate (11):
* TLSv1.3 (IN), TLS handshake, CERT verify (15):
* TLSv1.3 (IN), TLS handshake, Finished (20):
* TLSv1.3 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.3 (OUT), TLS handshake, Finished (20):
* SSL connection using TLSv1.3 / TLS_AES_128_GCM_SHA256
* ALPN: server accepted h2
* Server certificate:
*  subject: CN=rubygems.org
*  start date: Jul 30 17:45:18 2023 GMT
*  expire date: Aug 30 17:45:17 2024 GMT
*  subjectAltName: host "rubygems.org" matched cert's "rubygems.org"
*  issuer: C=BE; O=GlobalSign nv-sa; CN=GlobalSign Atlas R3 DV TLS CA 2023 Q3
*  SSL certificate verify ok.
* using HTTP/2
* h2h3 [:method: GET]
* h2h3 [:path: /api/v1/gems/nonexistentgem.json]
* h2h3 [:scheme: https]
* h2h3 [:authority: rubygems.org]
* h2h3 [user-agent: curl/8.0.1]
* h2h3 [accept: */*]
* Using Stream ID: 1 (easy handle 0x55a0807acb10)
> GET /api/v1/gems/nonexistentgem.json HTTP/2
> Host: rubygems.org
> user-agent: curl/8.0.1
> accept: */*
> 
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
< HTTP/2 404 
< content-type: text/plain; charset=utf-8
< x-frame-options: SAMEORIGIN
< x-xss-protection: 0
< x-content-type-options: nosniff
< x-download-options: noopen
< x-permitted-cross-domain-policies: none
< referrer-policy: strict-origin-when-cross-origin
< cache-control: no-cache
< content-security-policy: default-src 'self'; font-src 'self' https://fonts.gstatic.com; img-src 'self' https://secure.gaug.es https://gravatar.com https://www.gravatar.com https://secure.gravatar.com https://*.fastly-insights.com https://avatars.githubusercontent.com; object-src 'none'; script-src 'self' https://secure.gaug.es https://www.fastly-insights.com https://unpkg.com/@hotwired/stimulus/dist/stimulus.umd.js https://unpkg.com/stimulus-rails-nested-form/dist/stimulus-rails-nested-form.umd.js 'nonce-'; style-src 'self' https://fonts.googleapis.com; connect-src 'self' https://s3-us-west-2.amazonaws.com/rubygems-dumps/ https://*.fastly-insights.com https://fastly-insights.com https://api.github.com http://localhost:*; form-action 'self' https://github.com/login/oauth/authorize; frame-ancestors 'self'; report-uri https://csp-report.browser-intake-datadoghq.com/api/v2/logs?dd-api-key=pub852fa3e2312391fafa5640b60784e660&dd-evp-origin=content-security-policy&ddsource=csp-report&ddtags=service%3Arubygems.org%2Cversion%3A22625f39e4b3a6159635c2ffa688a88ef476ad6c%2Cenv%3Aproduction%2Ctrace_id%3A1142385850896100931
< x-request-id: 48dfce80-4095-451e-a49b-9f6b6538f374
< x-runtime: 0.012707
< strict-transport-security: max-age=31536000
< accept-ranges: bytes
< date: Tue, 31 Oct 2023 23:25:11 GMT
< via: 1.1 varnish
< age: 42
< x-served-by: cache-vie6342-VIE
< x-cache: HIT
< x-cache-hits: 1
< x-timer: S1698794712.622108,VS0,VE1
< vary: Accept-Encoding
< server: RubyGems.org
< content-length: 32
< 
* Connection #0 to host rubygems.org left intact

Can we close this for now or is there still need for pure JSON response? That would need most likely v2 API of this endpoint to keep it on super safe side.

[BanzaiMan]

I suppose it is an acceptable (if unfortunate) compromise. I hope v2 API would rectify this problem.