Use nested path parameters from the parent when using build

[chewi]

This removes a TODO! 😀

[chewi]

Rebased.

[chewi]

I've rebased this, refactored it slightly, and added a much-needed caching feature. I was surprised it didn't do that already.

[edtjones]

Thank you @chewi. Will review and merge as soon as poss. Haven't looked at the code yet; how are you caching? I ask because I would prefer that we don't have loads of partial solutions to caching floating around.

[chewi]

By caching, I mean that it sets the associated record using the setter method and by extension the @_her_association_foo instance variable that is already used for caching. Calling bar = foo.bars.build followed by bar.foo should not result in the foo record being fetched over HTTP again. ActiveRecord does not refetch from the database when you do the equivalent there, at least not when you use :inverse_of. I made it work even without :inverse_of here because the fetcher already does the same.

[chewi]

I've realised that my understanding was a little off before. @_her_association_foo is not used to cache the resource itself but the proxy. The fact that associations do not currently define a foo= method was throwing me off. I believe this change is still more or less valid but it won't work properly until I've made some further improvements elsewhere. Stay tuned.

[chewi]

I just about had things working nicely until I realised it breaks with request_new_object_on_build true. I am repeatedly being bitten by the fact that Her stores association data in the attributes hash. Except, of course, when it stores it in @cached_result. Why is this data stored in two places and what benefit is there in storing it in the attributes hash? It screws up things like change tracking and request parameters. ActiveRecord does not do it either.

I have wanted to write my own version of Her for a long while now but I will never have time to do that so I'm trying to make the best of what we have. Can we please discuss some of the questionable design decisions like this one?

[edtjones]

Happy to discuss @chewi - what would be the alternative, and how could we do it?

[chewi]

Since writing that, I've realised that Her::Model::ORM#build creates the request parameters directly from the given attributes hash rather than creating a new resource instance first so this problem is slightly different and I'll need to give it some more thought. This is what I want to do:

service = Service.find(1)
customer = service.customers.build
customer.service # This should not refetch the service.

I made this work by defining service= and allowing service to be assigned rather than just service_id. The problem is that when request_new_object_on_build true, the attributes hash includes service so you end up with a URL like /services/1/customers/new?service=%23%3CService:0x00000006f7ddb0%3E, which obviously isn't good. I could assign service_id first and then service afterwards but it is useful to have the cached service available when the resource is instantiated for callbacks and such.

My point above still stands though. I would like to just use @cached_result. When I try to take associations out of the attributes, tests like "Her::Model::Associations handling associations without details includes belongs_to relationship in params by default" start failing. It's not clear to me when associated data should or not should appear in request parameters. If associated data is included in a response, I don't think that should automatically mean that the data is included in a subsequent request. I was hoping you could shed some light on this.

[edtjones]

@chewi I would need to plough through the code to understand completely (sorry - didn't write this bit) but have you come across send_only_modified_attributes in the setup block? https://github.com/remiprev/her#dirty-attributes

[chewi]

Yes, I do use send_only_modified_attributes already but I don't think that matters? Modifying the associated data should not control whether it is included in the parent request or not. The API may not even support that. Using the above example:

class Service
  has_many :customers
end

class Customer
  belongs_to :service
end

service = Service.find(1)
customer = service.customers.build
customer.service.name = "bar"
customer.save # I wouldn't expect this to touch service. AR wouldn't in this case.

customer = Customer.find(1) # { name: "Bob", service: { name: "foo" } }
customer.service.name = "bar"
customer.save # I wouldn't expect this to touch service either.

[edtjones]

OK I see. Are these separate endpoints or nested data? I assume separate endpoints?

[chewi]

Separate endpoints in my case but I'm trying to consider the bigger picture here. I want to make Her work better for everyone.

[chewi]

I think I've finally got something workable now. I decided to exclude association data from build requests when the foreign key is present, e.g. if service_id is present then the service parameter will be excluded. It doesn't make much sense to send up association data for existing resources in a build request, even if it is modified.

There is further scope to include new associations in a build request but this would need data_key remappings as well as conversions via to_params. I don't need this so someone else can pick that up.

I'll give this stuff a bit more testing tomorrow before I push it up.

[chewi]

Okay, here it is. This goes quite a but further than the original pull request but the changes are at least loosely related and submitting them separately would have been far more hassle. Obviously there are a lot of changes here so it might be easier to focus on the specs to better understand why they are necessary.

[chewi]

IIRC this will conflict with the second commit of #298. This side should win but I can fix the conflict myself once you merge one of these.

[edtjones]

Really grateful for all your effort on this @chewi and the changeset looks like it's really useful. Paging @zacharywelch - I don't have a lot of headspace right now. Can you sign off on this?

[chewi]

I've made some relatively small tweaks to this but I'll give them a bit more testing before I push up again.

[zacharywelch]

Sorry overlooked this. @chewi let us know when you're ready and I'll give it a review. Thanks!

[chewi]

Small tweaks ballooned into bigger changes.

With the build requests working as I intended, some parameters used to build the path started appearing in the query string so I had to adjust build_request_path to optionally remove the parameters that were used.

Internal _ prefixed parameters found in nested resources were also leaking through so I had to ensure those were stripped out recursively rather than just at the top level.

The biggest change is the new "autosave" option. I had previously put hacks into my own gem to work around the fact that Her would nest associated resources in save requests, seemingly at random. This option now gives you control over this behaviour. I called it autosave because it is similar to the option found in ActiveRecord, where true always saves, false never saves, and nil only saves new records. I admit the name could be better though as people may think this saves associated resources in separate requests so I'm open to a different name. ActiveRecord defaults to nil but I have made true the default here to preserve existing behaviour. I have added a boat load of tests to make sure I got this right.

The Travis failures are expected because #298 now needs to be merged first.

[chewi]

Drat, after months of no issues and heavy use in production, my colleague ran into an infinite loop involving to_json. It looks very much like the earlier problem with inspect. Investigating...

[edtjones]

I’ve had this where an attribute is called ‘attributes’. Might not be related. I fixed up in a middleware. Ed

…

On 10 Apr 2018, at 16:21, James Le Cuirot ***@***.***> wrote: Drat, after months of no issues and heavy use in production, my colleague ran into an infinite loop involving to_json. It looks very much like the earlier problem with inspect. Investigating... — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[chewi]

It's not that. The new code does a better job of caching associated resources so you can easily end up with loops, hence why I had to fix this for inspect. This doesn't happen with to_json in ActiveRecord because it doesn't store associated records as regular attributes and doesn't otherwise traverse associations unless you explicitly include them.

I have since realised that it is my own code including the ActiveModel::Serialization::JSON module and I thought the problem might go away if I remove this. However, it actually makes the problem worse, causing both to_json and as_json to blow up. I'll see if I can make it behave more like ActiveRecord.

[chewi]

Fixed it by implementing something similar to what ActiveRecord has. I'll push it up tomorrow when we've tried it a bit more.

[chewi]

Bah, the existing include_root_in_json stuff is colliding with ActiveModel's include_root_in_json stuff. I'll try to work it out but I don't even use that.

[chewi]

I've resolved that by removing the Her getter/setter method in favour of ActiveModel's class attribute. The class attribute does not combine the getter/setter like Her does so I had to add a compatibility fix, though maybe we could change the API to make it consistent. This would be self.include_root_in_json = true instead of include_root_in_json true.

I haven't verified it but while working on this, I noticed that other similar variables like request_new_object_on_build may have a bug. It should only check the value for a parent class when the value for the current class is unset, not falsey. Using a class attribute would fix this and make it simpler but won't work for variables that have options like parse_root_in_json.

def request_new_object_on_build?
  @_her_request_new_object_on_build || (superclass.respond_to?(:request_new_object_on_build?) && superclass.request_new_object_on_build?)
end

[chewi]

I had barrels of fun rebasing this one following #491 but got there in the end. My concerns above about request_new_object_on_build and friends were actually addressed in #488 but this PR has now revealed that my compatibility fix is ineffective for subclasses. Still trying to figure out why but now it's the weekend. See you next week!

[chewi]

Rebased and dealt with the compatibility fix. It's quite nasty but it has to be because of how class_attribute works. I've tried very hard but you're welcome to come up with something better. I have tried to preserve existing behaviour in general but with the amount of change going on, it may be safer to do a major new release with a mention of possible breakage. Then we could simply drop the old syntax.

I had undo the "chop" change from #495 because it broke one of the tests. I believe this was actually a mistake as the method name will not always end with ? or =. In the test, _user_id was chopped down to _user_i.

[zacharywelch]

Good find @chewi 👏 I went ahead and took care of the chop on master. Will come back to your PR as soon as we finish the upgrade to rails 5.2