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

JR lamda relation_name gets broken #80

Closed
Justin-Maxwell opened this issue Aug 31, 2017 · 8 comments · Fixed by #81
Closed

JR lamda relation_name gets broken #80

Justin-Maxwell opened this issue Aug 31, 2017 · 8 comments · Fixed by #81

Comments

@Justin-Maxwell
Copy link

The ability to use context-driven relation names seems to be broken, because the context argument isn't passed as required to JSONAPI::Relationship.relation_name when invoked from within jsonapi-authorization.

relation_name - the name of the relation to use on the model. A lambda may be provided which allows conditional selection of the relation based on the context.

When JR invokes the lambda, the received argument looks something like this:
{:context=>{:current_user=>#<User id: 14, ... ...>, :demo_mode=>true, :action=>"show"}, :include_directives=>#<JSONAPI::IncludeDirectives:0x007f6b5d043290 @resource_klass=API::V1::SomeResource, @force_eager_load=false, @include_directives_hash={:include_related=>{:some_included=>{:include=>true, :include_related=>{}, :include_in_join=>true}}}>, :fields=>{}}

But when invoked from JSONAPI::Authorization::AuthorizingProcessor.authorize_include_item at authorize_include_item(resource_klass, source_record, rel_name), rather than passing the full context object, the lambda receives:
{:current_user=>#<Tibbing::User id: 14, ...>, :demo_mode=>true, :action=>"show"}

Which, since the context is at least present, could be cludged around, but unfortunately, it is then invoked from JSONAPI::Authorization::PunditScopedResource.fetch_relationship via v.relation_name({}) so we get {}, there is no way for the lamda to return the correct relation_name, and it errors out, disabling this functionality.

In our context, the demo_mode context boolean needs to 'pick' the appropriate result of a to_one relationship. I'm going to have another crack at over-riding records_for_ in the resource, but it would be good to ensure that the JR context is always provided to .relation_name, and test that lambda relation_names work 'as advertised' by JR.

I just wish I had the time to work on tweaking the gem and send through a pull-request, rather than just adding problems ... :-(

@valscion
Copy link
Member

Huh, interesting. Seems like JR gives us a different context in some different cases. I'll see if I can easily reproduce this

@valscion
Copy link
Member

Oh yeah, we do indeed have this one case where we're not using context like we should. We don't use it ourselves, so that's why we haven't come across this issue before.

@valscion
Copy link
Member

When JR invokes the lambda, the received argument looks something like this:

A-ha, so JR doesn't just pass the context, but it passes the entire options hash instead. That's.... weird. I thought the docs mentioned that only context part of that hash would be passed instead of the entire options hash 😱

@valscion
Copy link
Member

Seems like JR sometimes adds all sorts of things to the relationship. But when we're at the resource level, as is the case with JSONAPI::Authorization::PunditScopedResource, all we could get our hands on is the context.

https://github.com/cerebris/jsonapi-resources/blob/5af1f17068b66b0a17b7b69e5db334c640f6f34b/lib/jsonapi/processor.rb#L170-L184

I'll fix this by always sending a hash to the relationship accessors, that has the current context within a hash as {context: the_real_context}. Would that work for your use case?

@Justin-Maxwell
Copy link
Author

Yup - it's not actually our code that really needs it, but rather other JR functions that access it (that seem to have more to do with reverse lookup of the JR relationship object, I think). I have a working solution using records_for_ that works. I agree it's weird, I don't understand why JR passes the outer hash rather than just consistently passing the context.

JR having relationships, that have names, that may or may not be the relation_name, is sufficiently confusing that I'm going to try and avoid this until I get more comfortable with JR generally. It should have been 'association_name' I think. But I think your suggested approach will be of great help the next person who tries to use lambda relation_names.

@valscion
Copy link
Member

valscion commented Sep 4, 2017

Yeah it might be a bit confusing, I agree. Would you still be able to test out #81, please? This issue is an actual bug and I actually need your help to verify that my PR fixes your case.

@valscion
Copy link
Member

Oh well, sometimes we all have time pressure and can't help with OSS. No hard feelings! I have confidence that #81 fixed this bug, or at least it didn't make the matters worse 😄

@valscion
Copy link
Member

Fix for this landed in v1.0.0.alpha6, ping #48

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

Successfully merging a pull request may close this issue.

2 participants