-
-
Notifications
You must be signed in to change notification settings - Fork 253
Fix AR type cast to mirror quoting #422
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikolai-b thanks for the PR! I think this looks good. Was this only effecting prepared geometries? I thought there was some work done recently that added the SRID to the generated string in the rgeo-activerecord gem.
I suggested a similar PR #430 before finding yours. |
As of rgeo (3.0.1) and rgeo-activerecord (8.0.0) it does not look to be adding SRID. Rather, Re test assertions: From the testing I've done, the proposed changes look good and address the issue by using EWKB. (In my case, this was ultimately used with |
@inkstak thanks, I copied the logic from activerecord-postgis-adapter/lib/active_record/connection_adapters/postgis_adapter.rb Lines 122 to 130 in 5d380da
@keithdoggett I don't see any work in rgeo-activerecord around SRID. Given someone else has submitted a similar PR is there something either of us can do to aid in review (e.g. if you prefer the specs in the other PR)? Thanks |
Thanks you. I've included your spec as it now works, is that ok as it is your code? Sadly #431 is not fixed by this change |
It seems wrong that
uses the EWKB format but when the query is actually executed it uses
to_s
which uses the WKT without passing any SRID. This was causing me issues for me with preprepared statements but I couldn't create a simple text. I do think this gem should pass the SRID of bindings though.I'm not very happy with the test I've written here. I looked
.values_for_queries
or looking at the arel but the binds haven't been type_cast at this point so I've used explain.