-
-
Notifications
You must be signed in to change notification settings - Fork 256
Compatibility with Rails 8.1 #435
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
| # has_z: false | ||
| # has_m: false | ||
| def self.parse_sql_type(sql_type) | ||
| # Could be nil during type registration |
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.
Not entirely sure why this happens as I'm not fully familiar with active record internals, but this seems to work.
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.
I'm investigating it these days on crdb adapter, I hope I'll come with more information and a clean solution soonish :) (before the end of this month)
|
|
||
| def spatial_factory | ||
| @spatial_factory ||= | ||
| if frozen? |
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.
Debated on this branch for a bit, not sure when this object would be unfrozen now.
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.
I think we should see if there is a performance a space change, but I'd rather bubble that back up to the initialiser than create a factory every time. Also the if frozen? branch is useless since the goal of ractor compatibility for types as I understood is to have them frozen all the time !
|
There are more changed required to have the adapter working. For this reason, i decided to create an extension instead of legacy way of having a pg adapter.. My goal is to move that repo later to this organization. The interface is plug & play. |
|
Actually looking at the failed tests, they seem to all be from the ActiveRecord test suite itself. Is there a reason why the gem runs the tests for Rails? |
|
let upgrade to test with rc1 |
- Handle `cast_type` being the second parameter for `SpatialColumn.initialize`. - Handle nil case during OID initiation. - Handle case where in `spatial_factory` the object could be frozen for whatever reason. - Update CI to test against 8.1 and supported PG and Rubies.
5f6e071 to
6b0af9e
Compare
| gem "pg", "~> 1.0", platform: :ruby | ||
| gem "byebug" if ENV["BYEBUG"] | ||
|
|
||
| gem "rgeo-activerecord", git: "https://github.com/rgeo/rgeo-activerecord.git" |
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.
interesting!
Do rgeo need any release ?
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.
Yes, I had a PR for 8.1 support merged a few weeks ago..
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.
On it now ! Thank you for the PR
I found a solution for that in the 8.1 crdb adapter update cockroachdb/activerecord-cockroachdb-adapter@e656a24.
Running rails test suite ensure a complete coverage that we would not have with only our own test suite, and helps us make sure that we implement all API changes from the rails release we are bumping to |
|
Current state of things: Locally, tests should pass now with the inclusion of those backtrace fixes. I removed the I tried moving that logic into the initializer, but ran into two issues:
|
|
Looks like the runners that failed all failed on the same error. Which is an ActiveRecord test for fixtures. Possibly a flakey test? |
| ruby: ["3.4", "3.3", "3.2"] | ||
| # https://www.postgresql.org/support/versioning/ | ||
| pg: [12-master, 13-master, 14-master, 15-master, 16-master] | ||
| pg: [13-master, 14-master, 15-master, 16-master, 17-master] |
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.
pg 18 was released a month ago is there a reason not to include it here? my fault, this pr went up before it came out. still might be nice to add here though.
|
I'm a little confused... @t27duck is the path forward to adopt the new gem that @seuros is proposing (whereby the pg adapter is extended rather than replaced), or to bring in Rails 8.1 compatibility with this gem? We're blocked on our upgrade and would love assistance/guidance on where to go with this issue. |
This plan has not been discussed by maintainers of the gem, you'd have to ask @seuros. As far as I'm concern, I'd rather keep this gem. I'm currently working on rails compatibility for the CRDB adapter, once done I'll have time to handle this more properly. In the mean time, you can point to this branch. The differences are usually in some deep functionnalities and you're unlikely to have problems. And if you do, please report them here.
You can talk to your company about RGeo's open-collective and financing. Or directly hire me as a freelance if you need fast-paced guidance and proper deadlines. Otherwise, as we're few to maintain on our spare time, I cannot give you deadlines. |
|
@BuonOmo Actually i did send email to both you and @keithdoggett in 2024 i think. I did have a back and forth with Keith. My plan was to create it and transfer it to this org. I assumed you were in the same line as what Keith said, to keep this architecture. activerecord-postgis-adapter is an inherited adapter. I contributed to it when It was just basically a copy carbon the upstream adapter. I did raise my concern that one day it will be good have it extending AR instead of forking. activerecord-postgis is that wish but as reality, different archirecture, same api. I had that gem vendored for my needs since rails 7.0. but it had no README and was missing feature i didn't use. I contributed to this adapter, because i knew upgrade it, not because i needed the upgrade. When i released activerecord-postgis, i posted it in reddit, X, HN, and i got immediately hammered by Anti-AI people telling me that i vibed it and that it not how it should be made. They didn't try it or reviewed the code. They see Claude and emojis in readme... They went full tribal ape on me. The moderators deleted the post after it went -40 and subject became about AI slop. My tweet still up, but with minimal engagement. activerecord-postgis is the exact copy of this gem(api-wise), the underline Activerecord logic is from rgeo org. |
Never received it sorry, do you have the correct email ? buonomo dot ulysse at gmail dot com.
No-go for me, we're not a team large enough to handle such a transition, see all of the open issues and the time to upgrade to rails 8.1 as an indicator. I'm not taking one more project. I'd eventually be alright with mentioning it as an existing alternative in the README, if it is not a drop-and-replace but an alternative for a different use case.
I have a hard time understanding your point here. We already extend the PG adapter, which IMHO is a pretty correct way of doing. We don't copy, and I tend to try to patch rather than copy paste methods as much as I can, and would like to keep going in that direction.
You posted what?
Which post? About what topic? I have a bit of a tough time understanding the problems you are raising and how those are linked to your new gems.
Damn that's not cool. I really hope we can work out something that work for you as well. I'm all up for having a bigger team here, with better organisation. |
The alternative gem is a 99% drop-in replacement. The only difference is in annotation output and the use of postgresql:// instead of postgis://. (If you replace it and don’t fix the adapter or DATABASE_URL, it will crash.) My point is that I’ve been involved with this gem since the days when dazuma (the original author) was still maintaining it. Then came another era with Tee Perham, and we did a big refactor to inherit from the adapter, renamed columns to avoid conflicts with PostgreSQL internal types once Active Record started using them, that’s the current structure of this gem.
I posted about the other gem in social media 'Announcing activerecord-postgis, a drop-in replacement...' It got immediate downvotes and insults by email. (Most of them don’t even use Postgres or know what a spatial database is... maybe they thought I forked NASA or SpaceX.) I’m fine with just having a mention in the README for now. But my goal is for the other gem to eventually take this one’s place. I can maintain it, it’s smaller, cleaner, and has more tests. My only concern with solo maintenance is that it might work fine for my use case but break for others. Just to give you an example: before open-sourcing it, I had a hardcoded SRID. That works when I control the architecture, but I can’t open-source it and force everyone to use a single SRID. We need to get this PR green to ship. |
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.
Checked locally, the failure are just timing related. ![]()
I just checked again for the keywords
My other concern would also be that the day you decide to stop working on it, the whole community loses the gem... At least here we're two maintainers, and a few active developers.
Could you maybe make an issue in this repo explaining the main axes of differences and how we could refactor to get to an ideal place that keeps steps made here, history and still take advantage of the new rails architecture?
What do you mean? I believe there is still the ractor issue that we need to either tackle or create an issue for. And about the failing tests, if by timing you mean that they depend on test order, that is still not green IMHO. I've been using EDIT: I've also taken the time to look at your new adapter, and I see two main issues that make it a no go for me:
I'm not done with the review, this is quite a heavy chunk. But I can see the pieces we could migrate to this codebase, which would be nice. |
That exactly why i asked it to move it to I use the gem heavily, i did not see any regression. |
Then users can point to this branch with some sense of security. I would still focus on fixing tests and make sure ractor compatibility is correct. I would be okay with having an
Well, I'm up to opening PRs here to migrate some parts of your code (for instance the arel part, but I'm sure you know of others that would make sense). I still am against patching pg adapter and think we should rather inherit (as mentioned above), but that could be up for discussion. |
cast_typebeing the second parameter forSpatialColumn.initialize.spatial_factorythe object could be frozen for whatever reason.These changes require the changes from rgeo/rgeo-activerecord#83 so that gem is set to pull from the repo in this branch.
At least locally, there are 5 failures and 2 errors in the test suite with these changes. The failures are all in the deprecation tests and the errors are related to connection tests (I think).
I've been running both this branch and the changes to
rgeo-activerecordin an app for several weeks with no issue, though granted the app only really cares about storing and retrieving geometry data with minimal to no querying.If nothing else, here's a good starting point.