Skip to content
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

Create a new pull request by comparing changes across two branches #531

Merged
merged 17 commits into from
Apr 18, 2023

Conversation

GulajavaMinistudio
Copy link
Owner

Motivation / Background

This Pull Request has been created because [REPLACE ME]

Detail

This Pull Request changes [REPLACE ME]

Additional information

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

p8 and others added 17 commits April 15, 2023 21:53
Follow-up to #47916.

RDoc requires `<tt>` in this case.
Follow-up to #47933.

RDoc does not support backticks the way that Markdown does.  Instead,
inline code must be wrapped with `+` or `<tt>`.
This issue is hurting my brain a little, but I'll try and explain it. It's a regression introduced by #44290, in the following conditions:

- You use `minitest/spec` (either direction or via a wrapper like [`minitest-spec-rails`](https://github.com/metaskills/minitest-spec-rails))
- You run tests using a regex filter to identify which tests to run (either directly or via a wrapper like [`m`](https://github.com/qrush/m))

Since #44290 you could not execute a test that has a space in its name using the regex filter method. This is because:

- https://github.com/rails/rails/blob/444df0eee1b537ecaa11509e819b071d4e87b519/activesupport/lib/active_support/testing/declarative.rb#L6 doesn't define the `test` method if `Spec` is defined. Recall that the `test` method does 2 things: it defines the test, but it also replaces spaces with underscores in the test method name.
- In minitest specs, you use `it` to define tests. `it` [generates a test method name](https://github.com/minitest/minitest/blob/a9fa045044b4210cfd21a512b06d1a4527d709ba/lib/minitest/spec.rb#L223..L229) by taking user input and prepending `test_` and a number. It does *not* replace spaces with underscores. So the test method is defined as `"test_002_foo bar"`.
- Since #44290 when you provide the test runner a filter it replaces spaces with underscores before passing the filter onto minitest. For example, the filter might become `"/test_002_foo_bar/"`.
- Minitest thus can't find the test method and doesn't run it.

The issue is in `escape_declarative_test_filter`. Rather than converting spaces to underscores, it should match both spaces *and* underscores. This covers all use cases.

Co-authored-by: jonathanhefner <[email protected]>
* Docs: fix misspel on association_basics.md [ci-skip]

* Fix the same misspell on has_one_associations_test.rb
Document ActionText::RichText methods [ci-skip]
The [Trilogy database client][trilogy-client] and corresponding
[Active Record adapter][ar-adapter] were both open sourced by GitHub last year.

Shopify has recently taken the plunge and successfully adopted Trilogy in their Rails monolith.
With two major Rails applications running Trilogy successfully, we'd like to propose upstreaming the adapter
to Rails as a MySQL-compatible alternative to Mysql2Adapter.

[trilogy-client]: https://github.com/github/trilogy
[ar-adapter]: https://github.com/github/activerecord-trilogy-adapter

Co-authored-by: Aaron Patterson <[email protected]>
Co-authored-by: Adam Roben <[email protected]>
Co-authored-by: Ali Ibrahim <[email protected]>
Co-authored-by: Aman Gupta <[email protected]>
Co-authored-by: Arthur Nogueira Neves <[email protected]>
Co-authored-by: Arthur Schreiber <[email protected]>
Co-authored-by: Ashe Connor <[email protected]>
Co-authored-by: Brandon Keepers <[email protected]>
Co-authored-by: Brian Lopez <[email protected]>
Co-authored-by: Brooke Kuhlmann <[email protected]>
Co-authored-by: Bryana Knight <[email protected]>
Co-authored-by: Carl Brasic <[email protected]>
Co-authored-by: Chris Bloom <[email protected]>
Co-authored-by: Cliff Pruitt <[email protected]>
Co-authored-by: Daniel Colson <[email protected]>
Co-authored-by: David Calavera <[email protected]>
Co-authored-by: David Celis <[email protected]>
Co-authored-by: David Ratajczak <[email protected]>
Co-authored-by: Dirkjan Bussink <[email protected]>
Co-authored-by: Eileen Uchitelle <[email protected]>
Co-authored-by: Enrique Gonzalez <[email protected]>
Co-authored-by: Garrett Bjerkhoel <[email protected]>
Co-authored-by: Georgi Knox <[email protected]>
Co-authored-by: HParker <[email protected]>
Co-authored-by: Hailey Somerville <[email protected]>
Co-authored-by: James Dennes <[email protected]>
Co-authored-by: Jane Sternbach <[email protected]>
Co-authored-by: Jess Bees <[email protected]>
Co-authored-by: Jesse Toth <[email protected]>
Co-authored-by: Joel Hawksley <[email protected]>
Co-authored-by: John Barnette <[email protected]>
Co-authored-by: John Crepezzi <[email protected]>
Co-authored-by: John Hawthorn <[email protected]>
Co-authored-by: John Nunemaker <[email protected]>
Co-authored-by: Jonathan Hoyt <[email protected]>
Co-authored-by: Katrina Owen <[email protected]>
Co-authored-by: Keeran Raj Hawoldar <[email protected]>
Co-authored-by: Kevin Solorio <[email protected]>
Co-authored-by: Leo Correa <[email protected]>
Co-authored-by: Lizz Hale <[email protected]>
Co-authored-by: Lorin Thwaits <[email protected]>
Co-authored-by: Matt Jones <[email protected]>
Co-authored-by: Matthew Draper <[email protected]>
Co-authored-by: Max Veytsman <[email protected]>
Co-authored-by: Nathan Witmer <[email protected]>
Co-authored-by: Nick Holden <[email protected]>
Co-authored-by: Paarth Madan <[email protected]>
Co-authored-by: Patrick Reynolds <[email protected]>
Co-authored-by: Rob Sanheim <[email protected]>
Co-authored-by: Rocio Delgado <[email protected]>
Co-authored-by: Sam Lambert <[email protected]>
Co-authored-by: Shay Frendt <[email protected]>
Co-authored-by: Shlomi Noach <[email protected]>
Co-authored-by: Sophie Haskins <[email protected]>
Co-authored-by: Thomas Maurer <[email protected]>
Co-authored-by: Tim Pease <[email protected]>
Co-authored-by: Yossef Mendelssohn <[email protected]>
Co-authored-by: Zack Koppert <[email protected]>
Co-authored-by: Zhongying Qiao <[email protected]>
`ActiveSupport::MessagePack` is a serializer that integrates with the
`msgpack` gem to serialize a variety of Ruby objects.  `AS::MessagePack`
supports several types beyond the base types that `msgpack` supports,
including `Time` and `Range`, as well as Active Support types such as
`AS::TimeWithZone` and `AS::HashWithIndifferentAccess`.

Compared to `JSON` and `Marshal`, `AS::MessagePack` can provide a
performance improvement and message size reduction.  For example, when
used with `MessageVerifier`:

  ```ruby
  # frozen_string_literal: true

  require "benchmark/ips"
  require "active_support/all"
  require "active_support/message_pack"

  marshal_verifier = ActiveSupport::MessageVerifier.new("secret", serializer: Marshal)
  json_verifier = ActiveSupport::MessageVerifier.new("secret", serializer: JSON)
  asjson_verifier = ActiveSupport::MessageVerifier.new("secret", serializer: ActiveSupport::JSON)
  msgpack_verifier = ActiveSupport::MessageVerifier.new("secret", serializer: ActiveSupport::MessagePack)

  ActiveSupport::Messages::Metadata.use_message_serializer_for_metadata = true
  expiry = 1.year.from_now
  data = { bool: true, num: 123456789, string: "x" * 50 }

  Benchmark.ips do |x|
    x.report("Marshal") do
      marshal_verifier.verify(marshal_verifier.generate(data, expires_at: expiry))
    end

    x.report("JSON") do
      json_verifier.verify(json_verifier.generate(data, expires_at: expiry))
    end

    x.report("AS::JSON") do
      asjson_verifier.verify(asjson_verifier.generate(data, expires_at: expiry))
    end

    x.report("MessagePack") do
      msgpack_verifier.verify(msgpack_verifier.generate(data, expires_at: expiry))
    end

    x.compare!
  end

  puts "Marshal size: #{marshal_verifier.generate(data, expires_at: expiry).bytesize}"
  puts "JSON size: #{json_verifier.generate(data, expires_at: expiry).bytesize}"
  puts "MessagePack size: #{msgpack_verifier.generate(data, expires_at: expiry).bytesize}"
  ```

  ```
  Warming up --------------------------------------
               Marshal     1.206k i/100ms
                  JSON     1.165k i/100ms
              AS::JSON   790.000  i/100ms
           MessagePack     1.798k i/100ms
  Calculating -------------------------------------
               Marshal     11.748k (± 1.3%) i/s -     59.094k in   5.031071s
                  JSON     11.498k (± 1.4%) i/s -     58.250k in   5.066957s
              AS::JSON      7.867k (± 2.4%) i/s -     39.500k in   5.024055s
           MessagePack     17.865k (± 0.8%) i/s -     89.900k in   5.032592s

  Comparison:
           MessagePack:    17864.9 i/s
               Marshal:    11747.8 i/s - 1.52x  (± 0.00) slower
                  JSON:    11498.4 i/s - 1.55x  (± 0.00) slower
              AS::JSON:     7866.9 i/s - 2.27x  (± 0.00) slower

  Marshal size: 254
  JSON size: 234
  MessagePack size: 194
  ```

Additionally, `ActiveSupport::MessagePack::CacheSerializer` is a
serializer that is suitable for use as an `ActiveSupport::Cache` coder.
`AS::MessagePack::CacheSerializer` can serialize `ActiveRecord::Base`
instances, including loaded associations.  Like `AS::MessagePack`, it
provides a performance improvement and payload size reduction:

  ```ruby
  # frozen_string_literal: true

  require "benchmark/ips"
  require "active_support/message_pack"

  ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")

  ActiveRecord::Schema.define do
    create_table :posts, force: true do |t|
      t.string :body
      t.timestamps
    end

    create_table :comments, force: true do |t|
      t.integer :post_id
      t.string :body
      t.timestamps
    end
  end

  class Post < ActiveRecord::Base
    has_many :comments
  end

  class Comment < ActiveRecord::Base
    belongs_to :post
  end

  post = Post.create!(body: "x" * 100)
  2.times { post.comments.create!(body: "x" * 100) }
  post.comments.load
  cache_entry = ActiveSupport::Cache::Entry.new(post)

  Rails70Coder = ActiveSupport::Cache::Coders::Rails70Coder
  CacheSerializer = ActiveSupport::MessagePack::CacheSerializer

  Benchmark.ips do |x|
    x.report("Rails70Coder") do
      Rails70Coder.load(Rails70Coder.dump(cache_entry))
    end

    x.report("CacheSerializer") do
      CacheSerializer.load(CacheSerializer.dump(cache_entry))
    end

    x.compare!
  end

  puts "Rails70Coder size: #{Rails70Coder.dump(cache_entry).bytesize}"
  puts "CacheSerializer size: #{CacheSerializer.dump(cache_entry).bytesize}"
  ```

  ```
  Warming up --------------------------------------
          Rails70Coder   329.000  i/100ms
       CacheSerializer   492.000  i/100ms
  Calculating -------------------------------------
          Rails70Coder      3.285k (± 1.7%) i/s -     16.450k in   5.008447s
       CacheSerializer      4.895k (± 2.4%) i/s -     24.600k in   5.028803s

  Comparison:
       CacheSerializer:     4894.7 i/s
          Rails70Coder:     3285.4 i/s - 1.49x  slower

  Rails70Coder size: 808
  CacheSerializer size: 593
  ```

Co-authored-by: Jean Boussier <[email protected]>
Introduce adapter for Trilogy, a MySQL-compatible DB client
Fix regression in test name filters with specs
`connection.check_version` will emit a query if it doesn't have
a connection pool on which to grab a SchemaCache.
Active Record: assign connection pool before checking version
Use Trilogy#discard! when discard! called on TrilogyAdapter
@GulajavaMinistudio GulajavaMinistudio merged commit 8dfabd0 into GulajavaMinistudio:main Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants