chore(ci): switch to github actions

[BuonOmo]

When this project was first started, Github Actions were not an option. Now that they are, and that teamcity is less accessible (lesser used, needs connection, ci link is not up to date), we are switching to gh actions.

Fixes #279

[BuonOmo]

so we can run bundler tasks from any subdirectory

[BuonOmo]

fixes tests as ruby 3 stopped interpreting keyword args as normal args

[BuonOmo]

fixes tests, psych got more strict over the years

[BuonOmo]

This is taken from https://github.com/rails/rails/blob/7f338b9082fe3488f681c171b93c371cfc951f22/load_paths.rb.

With the previous version we would load the railties/lib/rails.rb file from rails. This file defines Rails.env. And having this method defined keep a cached version of the env, failing tests that are trying to redefine the rails env...

[rafiss]

nice improvement

[BuonOmo]

As of now, here's the test summary result: 8015 runs, 25224 assertions, 12 failures, 263 errors, 29 skips.

And using the command:

grep -EA 2 ' (Error|Failure):$' test.txt | awk 'NR%4==3' | cut -d' ' -f2- | sort | uniq -c | sort

we get a good information on what test to fix first!

      1 'p_paperbacks_s' view should exist
      1 ActiveRecord::ConnectionAdapters::PostgreSQL::Quoting::IntegerOutOf64BitRange expected but nothing was raised.
      1 Expected "id" to be nil.
      1 Expected /"latlon":"POINT\s\(1\.0\s2\.0\)"/ to match "{\"id\":null,\"coordinates\":null,\"latlon\":\"POINT (1 2)\",\"boundary\":null,\"m_poly\":null,\"points\":null,\"path\":null,\"shape\":null}".
      1 Expected /= '1' \/\* foo \*\// to match "SELECT \"posts\".* FROM \"posts\" WHERE \"posts\".\"id\" = '1' /* ** //foo// ** */".
      1 Expected false to be truthy.
      1 Expected: "9223372036854775807"
      1 Expected: "9223372036854775808"
      1 Expected: 80000
      1 Expected: ["p_paperbacks_s"]
      2 Expected 2010-01-01 11:00:00 UTC to be an instance of ActiveSupport::TimeWithZone, not Time.
      2 RuntimeError: Wrapped undumpable exception for: ActiveRecord::StatementInvalid: PG::FeatureNotSupported: ERROR:  at or near "(": syntax error: unimplemented: this syntax
      3 ActiveRecord::StatementInvalid: PG::InFailedSqlTransaction: ERROR:  current transaction is aborted, commands ignored until end of transaction block
      3 RuntimeError: Wrapped undumpable exception for: ActiveRecord::StatementInvalid: PG::UndefinedTable: ERROR:  relation "p_paperbacks_s" does not exist
     14 NoMethodError: undefined method `each' for nil
     26 IOError: not opened for writing
    244 ActiveRecord::Fixture::FixtureError: table "accounts" has no columns named "status".

[BuonOmo]

I have to do some more documentation for this. Maybe @rafiss you know where to dig?

My current understanding of the context is that in ActiveRecord it seems that there is a 64bit cap for integer with the pg adapter. But since ints are stringified in crdb, there is no such thing for us.

[rafiss]

i'm surprised - it shouldn't be possible to have an integer wider than 64 bits in CRDB.

[BuonOmo]

Ok I'll dig further then

[BuonOmo]

So what I understood from this test: when quoting in pg, activerecord checks if the int is withing the 64 bit range with the #check_int_in_range method. The error message related to that is:

        def check_int_in_range(value)
          if value.to_int > 9223372036854775807 || value.to_int < -9223372036854775808
            exception = <<~ERROR
              Provided value outside of the range of a signed 64bit integer.


              PostgreSQL will treat the column type in question as a numeric.
              This may result in a slow sequential scan due to a comparison
              being performed between an integer or bigint value and a numeric value.


              To allow for this potentially unwanted behavior, set
              ActiveRecord.raise_int_wider_than_64bit to false.
            ERROR
            raise IntegerOutOf64BitRange.new exception
          end
        end

https://github.com/rails/rails/blob/4e01a7e8753c30e22264e185082524a9ea5e0c90/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb#L28-L42

So PG would think of strings containing only numbers as int, and would be way slower if they exceed 64bit. @rafiss does this ring a bell to you? Is there a similar behaviour in cockroach?

I'll keep looking for documentation related to that in pg to be sure. Might edit this message when I find an answer.

The fix, if need be, is fairly easy.

EDIT: I found the related CVE GHSA-579w-22j4-4749

EDIT 2: The adapter enforces quoting numbers as string as mentionned here:

        # CockroachDB does not allow inserting integer values into string
        # columns, but ActiveRecord expects this to work. CockroachDB will
        # however allow inserting string values into integer columns. It will
        # try to parse string values and convert them to integers so they can be
        # inserted in integer columns.
        #
        # We take advantage of this behavior here by forcing numeric values to
        # always be strings. Then, we won't have to make any additional changes
        # to ActiveRecord to support inserting integer values into string
        # columns.

So it seems like we don't really have the 64 bit issue the same way it impacts pg. But we might have a similar issue anyway. I'm not very familiar with cockroach, but if you want to pair up on that I think we could ensure that everything is ok quickly. Here's an article detailing the CVE https://code.jeremyevans.net/2022-11-01-forcing-sequential-scans-on-postgresql.html

[rafiss]

ah i understand now - so if it's larger than 64 bits it's treated as decimal (also called numeric) rather than integer

i think matching PG here makes sense. it's fine to make that change later too.

[rafiss]

thanks for the nice research on this!

[BuonOmo]

@rafiss sorry didn't see your message before editing. I think this needs our attention!

[BuonOmo]

After investigating some more, this adapter is not affected by the mentioned vulnerability. When quoting a number using this gem, it will be converted to a string first, taking advantage of cockroach's behavior of converting implicitly strings into numbers when needed. That conversion done on the server side is checking for bounds and raising if need be.

Here's an example of some code that could be abused in the pg adapter before the patch, but that raises in this adapter:

# age is an int column with an index
Cat.where("age = ?", 2**63)
# => PG::InvalidTextRepresentation: ERROR:  could not parse "9223372036854775808" as type int: strconv.ParseInt: parsing "9223372036854775808": value out of range (ActiveRecord::StatementInvalid)

[BuonOmo]

@rafiss I've fixed loads of tests, some are still present but I think I'd rather merge this and focus on a new feature for the next week, and I'll see flaky tests coming at the same time! WDYT?

[rafiss]

i'm glad to see these improvements! and yes, let's merge this now, and we can make incremental progress on it

i'm not sure about the 64-bit test. should we understand it better before merging, or can we do that later too?