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

Fix error where polymorphic relation not set #54

Closed
wants to merge 1 commit into from

Conversation

SebastianEdwards
Copy link

Currently if you declare a polymorphic association on a resource AuthorizingProcessor.related_models will error out if you try to create a record without having that relation set. The assoc_value will be nil which is not a Hash so the polymorphic relationship code route is not be taken. This fixes that.

Currently if you declare a polymorphic association on a resource `AuthorizingProcessor.related_models` will error out if you try to create a record without having that relation set. The `assoc_value` will be `nil` which is not a `Hash` so the polymorphic relationship code route is not be taken. This fixes that.
@valscion
Copy link
Member

Thanks! Are you able to come up with a test scenario for this? We've one polymorphic association in the spec dummy app, Article#tags, so maybe it could be leveraged in some spec to make sure we don't break these polymorphic associations in the future either?

@valscion
Copy link
Member

Hiya @SebastianEdwards, are you able to show me an example of a JSONAPI::Resource class with a request that fails without your fix? I didn't quite catch what you were trying to say in your PR description.

If you could provide an example with this level of detail, that would be super awesome ☺️

@valscion
Copy link
Member

valscion commented Apr 5, 2017

Ping @SebastianEdwards ☝️ I'm waiting for your answer before I'm able to figure this out.

@SebastianEdwards
Copy link
Author

Cool - will come back with a test soon. Bit swamped w/ work at the moment!

@valscion
Copy link
Member

Hiya @SebastianEdwards, still waiting here ☺️. If you don't have the time to create a test, would you be able to add some more details to what kind of resource this fixes? Something like this would be amazing to have:

Here's the setup:

Comment.new(id: 1, reviewing_user: user_with_id_2).save
class CommentResource < JSONAPI::Resource
  include JSONAPI::Authorization::PunditScopedResource

  has_many :tags
  has_one :article
  has_one :reviewer, relation_name: "reviewing_user", class_name: "User"
end

And we'd send this JSON API request:

PATCH /comments/1

"data": {
  "type": "articles",
  "id": "1",
  "relationships": {
    "reviewer": {
      "data": {
        "type": "user",
        "id": "3"
      }
    }
  }
}

@jpalumickas
Copy link
Contributor

jpalumickas commented May 26, 2017

@valscion

There is also a problem when we have has_one relationship and trying to save it with parent but sending null

class User
  belongs_to :nationality, class_name: 'Country'
end

PATCH /users/1

"data": {
  "type": "users",
  "id": "1",
  "relationships": {
    "nationality": null
    }
  }
}

So we need to add same functionality (when nil) to related_models_with_context also in 1.0.0.alpha3

now we have an error:

{
  "errors": [
    {
      "title": "Record not found",
      "detail": "The record identified by  could not be found.",
      "code": "404",
      "status": "404"
    }
  ]
}

@valscion
Copy link
Member

Released v1.0.0.alpha4 with the fix to @jpalumickas' issue with the PATCH request. I'm afraid the issue that @SebastianEdwards has is still a mystery to me, as I haven't received the details I need to understand what this issue is about.

@valscion
Copy link
Member

valscion commented Jul 4, 2017

@SebastianEdwards, still waiting for more details on your issue. I'd love if we could fix the issue you're facing and have a spec ensuring we don't break it in the future.

See #54 (comment) for what I'd need to understand what this issue is about.

@SebastianEdwards
Copy link
Author

Hi @valscion,

Sorry I've been such a slack bastard with this. If only good intentions could write tests!

My issue was exactly what @jpalumickas had described - my front-end code would send null in the place of a { "data": { "id": 1, "type": "thing" } } object rather than just leaving the key off entirely. This null wasn't handled in the processor which expected a hash.

Looks like the fix has been merged in another PR with tests so happy days! (Thanks @jpalumickas!)

@valscion
Copy link
Member

valscion commented Jul 5, 2017

Can you confirm that your case is solved by the fix done in #69?

@SebastianEdwards
Copy link
Author

Yup. #69 fixes the issue - will close this.

@valscion
Copy link
Member

valscion commented Jul 6, 2017

Thanks for following up with the confirmation! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants