Query can end up doing a POST!

[crayment]

Using latest alpha (master) I have the following query:

Parse::Query.new('QuizUserGrade').tap do |q|
  q.value_in('groupMembership', group_memberships.map(&:pointer))
end

Which works fine until you have too many group_memberships. (breaks with 25, works with 10). Once you have too many it does a POST instead of a GET and you end up creating a record when you are trying to execute a query. Still digging but this is a bad bug.

[crayment]

I think it's related to the Faraday::GetMethodOverride option. Parse isn't handling this and is instead creating an object with a where attribute.

[rhymes]

@crayment can you contribute a patch or at least provide more detail about the issue? Thank you

[crayment]

I think technically this is a bug with open source parse not respecting the X-Http-Method-Override header. This leads to the nasty behaviour of it just seeing a POST with a giant where attribute and so it creates an object.

I resolved this for now by disabling the GetMethodOverride middleware by doing client.session.builder.delete Faraday::GetMethodOverride.

I would suggest removing this middleware (delete this line) until it's confirmed that parse is correctly handling the header. This will likely bring back issues with large GET requests but is much better than the alternative at this point.

[rhymes]

@crayment have a look at this PR, let me know if it's satisfactory for you. Instead of outright deleting the middleware I just made it so you can disable it.

[crayment]

@rhymes #211 Looks great and gives users a good way to resolve the issue, thanks! The only thing I would change is defaulting to false (at least when a custom host is specified?) but your judgement is better here on what users would expect.

[rhymes]

@crayment I agree with you, it should default to false, the issue though is that it might break existing user's code that's relying on the, now, implicit override switch.

@xavdid what do you think about this issue?

[xavdid]

Nice!

I think we should default it to true because we don't want to be changing people's code out from under them. If they're having an issue (like you were) we've given them an avenue to solve it, but their exiting code won't change unexpectedly.

Related, we should bump the version number when we make changes. Even though we're not releasing to rubygems, bundler will still respect the version number and when there are actual changes like this, it'd be good to signify that somehow. I would think this would be a minor release.

Otherwise, looks awesome.

[rhymes]

@xavdid you mean false then ? false would be the current behavior (with the potential problem of override ending up a post for large GETs which would be fixed by setting the flag as true).

I agree wth the version bump, I'll add it to the PR

[xavdid]

@rhymes hmm? If I'm reading the change correctly, c.use Faraday::GetMethodOverride currently runs each time now. Functionally, c.use Faraday::GetMethodOverride if true is identical, so true should be the default so we don't change the code that relies on this bug.

The option to fix it is good, and we can default it the other way if we do a major release (and really in most cases, it shouldn't matter).

[rhymes]

@xavdid yeah sorry, tired eyes! thanks for the patience.

I'll merge the PR