Gem render consistency

[nicolas-besnard]

Hi,

It appears that the Gem doesn't use the same json render everywhere. Do you think it can be a good idea to generate render with template file such as jBuilder or with rabl ?

[lynndylanhurley]

@nicolas-besnard - that might be a good idea. Just for clarification, can you describe some of the inconsistencies?

[nicolas-besnard]

You missed a success: false here and here

And a success: true right here

[nicolas-besnard]

And you send a hash in errors here instead of an array :)

[lynndylanhurley]

The error response format is intentional (see #44). The idea is that it's easier to check a hash for matches to field names than an array. For example:

<fieldset>
  <div>
    <label>Email</label>
    <p class="errors" ng-show="errors.messages['email']">Email {{ errors.messages['email'] }}</p>
    <input type="text" name="email" ng-model="form.email" />
  </div>
</fieldset>

In the above example, the error message is set to show if the email key is defined on the error object. This would be more difficult if the errors were in an array.

There is a property of the error hash called full_messages that contains the errors in an array format.

So an error response should look like this:

{
  data: {
    errors: {
      // messages hash contents
      full_messages: [
        // full_messages array contents
      ]
    },
  }
}

As for the missing success responses - should we introduce a JSON rendering library, or should we just fix the inconsistencies? I'm asking seriously - what are the pros and cons of each fix?

[nicolas-besnard]

Well, I think a JSON rendering library is a better solution because you can use your own which fit your project.

I personally use rabl in my job, and I kind of like it :) And if you use it with oj, it's faster than a render json

[nicolas-besnard]

I understand your point about the field name. But in the case you use this Gem in an API with an iPhone Application, I don't think this is required.

Moreover, consistency in a REST API is really important. You should have an array for errors but add an other key on the response for this specific case. This is my personal opinion as an API and iOs developper.

[lynndylanhurley]

You should have an array for errors but add an other key on the response for this specific case.

That was my initial impulse as well, and I think Rails handles the errors that way internally.

@jasonswett actually fixed up the error responses in #50. @jasonswett - do you want to chime in here?

[nicolas-besnard]

That's why a JSON rendering library may be a good idea here. Everyone can chose his render to fit his project :)

[jasonswett]

I feel like all the response code in that controller could use a refactor to DRY it up, and maybe introducing a JSON rendering library at the same time would make sense.

I understand Rails 4 defaults to Jbuilder and Rails::API defaults to ActiveModel::Serializers. Seems like we would want to swim with the current and use one of those two if we use anything. I don't have anything against RABL as a technology but it doesn't seem to be a de-facto standard. I'm also not even sure if introducing a JSON rendering library is the way to go at all. Maybe all we need is to clean up duplication.

I haven't taken a look at this issue under a microscope yet so I'm not actually sure my thoughts here even have any validity. Those are just my off-the-cuff thoughts, and I'm ready to be totally wrong.

[nicolas-besnard]

Benchmarks show that Rabl is faster than Jbuilder. Again, a JSON rendering library seems to be really important for personalization. I personally need it for my project. You can't have a JSON format for your authentification and an other one for the rest of your API.

[lynndylanhurley]

@nicolas-besnard - ok I'm convinced!

I won't have time to work on this until early January. But it sounds like you're an expert in this area - is this something that you'd like take on?

[nicolas-besnard]

Sure !

[lynndylanhurley]

Cool, send me a PR and I'll merge ASAP!

[nicolas-besnard]

Are you of with someting like that :

In registration_controller.rb

unless params[:confirm_success_url]
   @render = OpenStruct.new
   @render.status = 'error'
   @render.data = @resource
   @render.errors = ["Missing `confirm_success_url` params."]

   return render 'devise_token_auth/registrations/missing_confirm_success_param', status: 403
end

in views/devise_auth_token/missing_confirm_success_param.json.rabl:

object @render
attributes :status, :data, :errors

Hum ... I'm not sure it's the right think to do. I usually don't use Rabl to render "custom" JSON, I use it with Model.

Here, we have to instantiate OpenStruct which appears to be slow sometimes has you can see here. But the view is render is 0.5ms.

I tried to use `.json.erb, which work out of the box, but the view was render in 10.5ms.

However, I can't see any other solutions to allow users to use custom render.

Any idea ?

EDIT : I just made a benchmark. The results are on the bottom. Hashi seems to be 2x faster than OpenStruct on my i5.

[lynndylanhurley]

The code that you posted looks good to me.

And I like Hashi. If you think it will help us, feel free to include it as a dependency.

[nicolas-besnard]

Good, I will continue tomorrow :)

[nicolas-besnard]

Will preparing my PR, I note an other inconsistencies.

status: 'error' is used in RegistrationsContoller, but success: false is used in PasswordsController.

I prefer the success: false or success: true, which is easier to test (and faster than comparing a string)

[nicolas-besnard]

@lynndylanhurley : What do you think of my last comment ?

[lynndylanhurley]

@nicolas-besnard - the more I think about it, the more I wonder if we need the status / success params. We're already returning status codes for each response (200, 401, 402, 500, etc). That should be enough, right?

[nicolas-besnard]

Yes, you're right. I usually use status code + body to determine the action to do in front applications.

[nicolas-besnard]

I'm busy since the beginning of the year. I'll give some of my time on this project when my current company project will be finished.

[nicolas-besnard]

I'm free this week-end. Should I implement your solution ? Or do you come with an other alternative ?

[nicolas-besnard]

What about a renderer function on the user concern ? Replace this with the following code in the concern :

render render json: @resource.create_session_render

For example, here's the output of my app on Session#create

{
    "user": {
        "id": 19,
        "first_name": null,
        "last_name": null,
        "email": "besnard.nicolas@gmail.com",
        "created_at": 1424020765,
        "updated_at": 1424020765,
        "birthday": null,
        "gender": null,
        "username": "Clara Rowe",
        "photos": {
            "host": "https://mybucket.s3.amazonaws.com/",
            "profile": null
        }
    }
}

So far, I reimplement each class manually to do that.

[booleanbetrayal]

Closing due to inactivity / addressed PR ... can re-open if need be.

[itkin]

hello, just found your gem and i'm giving it a try, this render issue bugs me so ended here ... so my point of vue, i guess you have some contraints from client side & oauth2 lib, but :

• the success & status booleans seem unusefull since you got this information from the response status (client side the promise resolve or fail consequently)
• don't understand why to return the data sent to the serialized resource to the client, it knows the data it just sent plus you may expose some attributes you don't want to (using simple as_json)
• there is still some as_json calls in registration controller (which is inconsistent with the custom method set from the user concern)

So to sum up, why not drop all this success / status / data keys in the responses and

• on success just return the resource using rails (ex-)standard and devise current respond_with approach. It let the possibility to the user to use his own renderer by adding a view file and it defaults on as_json
• on error just the error list (with the key :errors) with no data

If you think it may be interesting i can PR something

[Animeye]

I'm also curious what happened with the render cleanup/simplification that was discussed in this issue. I see in the PR that @nicolas-besnard mentioned making a new PR, but it doesn't look like anything actually happened with that