diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 0bc2b798e..dc2c14700 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -46,6 +46,10 @@ jobs: - build_part: "run_datastore_specs" ruby: "4.0" datastore: "opensearch:2.19.0" + # We have a special build part for JRuby. + - build_part: "run_specs_for_jruby" + ruby: "jruby-10.0" + datastore: "elasticsearch:9.2.4" # Other build parts run on max Ruby and primary datastore only. - build_part: "run_misc_checks" ruby: "4.0" diff --git a/CLAUDE.md b/CLAUDE.md index 05c2ebc4a..ff5b12896 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -156,6 +156,7 @@ Custom gems can be added via `Gemfile-custom` (see `Gemfile-custom.example`), th - Always put a blank space after `#` in comments. This applies to all comments, including RBS type annotation comments (e.g., `# : String` not `#: String`). - Avoid defensive code for impossible cases. Don't add checks, tests, or handling for scenarios that should never occur due to the design of the system. If something can only happen due to a bug in the implementation, it's better to fail fast than to silently handle it. - Drop unnecessary namespace prefixes. When code is already inside `module ElasticGraph`, use `Indexer` instead of `::ElasticGraph::Indexer`, and `Errors::ConfigError` instead of `::ElasticGraph::Errors::ConfigError`. Use fully qualified names only when necessary to avoid ambiguity. +- Prefer using `prepend` + `super` rather than `alias_method` when needing to hook in and override a method. ### RBS Type Signatures diff --git a/Steepfile b/Steepfile index e7ba1cfcc..8cad1fff5 100644 --- a/Steepfile +++ b/Steepfile @@ -66,6 +66,11 @@ target :elasticgraph_gems do elasticgraph-schema_definition/lib/elastic_graph/schema_definition/schema_elements/union_type.rb ]) + # We don't bother type checking JRuby patches. + ignore(*%w[ + **/lib/elastic_graph/*/jruby_patches.rb + ]) + library "logger", "base64", "date", diff --git a/elasticgraph-graphql/lib/elastic_graph/graphql/jruby_patches.rb b/elasticgraph-graphql/lib/elastic_graph/graphql/jruby_patches.rb new file mode 100644 index 000000000..ddbec7252 --- /dev/null +++ b/elasticgraph-graphql/lib/elastic_graph/graphql/jruby_patches.rb @@ -0,0 +1,32 @@ +# Copyright 2024 - 2026 Block, Inc. +# +# Use of this source code is governed by an MIT-style +# license that can be found in the LICENSE file or at +# https://opensource.org/licenses/MIT. +# +# frozen_string_literal: true + +# Central location for JRuby workarounds in the graphql gem. +# Each patch should reference the upstream fix and specify when it can be removed. + +# Bug: graphql-ruby's `Dataloader#with` has two branches: +# - MRI Ruby 3+: `def with(source_class, *batch_args, **batch_kwargs)` -- properly separates kwargs +# - Non-MRI or Ruby < 3: `def with(source_class, *batch_args)` -- collapses kwargs into a Hash in batch_args +# +# JRuby 10 targets Ruby 3.4 compatibility and handles **kwargs correctly, but +# graphql-ruby excludes all non-MRI engines due to a TruffleRuby issue. +# This causes `Source.new` to receive a positional Hash instead of keyword args, +# breaking any Source subclass that uses keyword-only parameters in `initialize`. +# +# Reported upstream: https://github.com/rmosolgo/graphql-ruby/issues/5541 +# TODO: remove once graphql-ruby supports JRuby in the kwargs branch. +::GraphQL::Dataloader.class_exec do + def with(source_class, *batch_args, **batch_kwargs) + batch_key = source_class.batch_key_for(*batch_args, **batch_kwargs) + @source_cache[source_class][batch_key] ||= begin + source = source_class.new(*batch_args, **batch_kwargs) + source.setup(self) + source + end + end +end diff --git a/elasticgraph-graphql/lib/elastic_graph/graphql/resolvers/nested_relationships_source.rb b/elasticgraph-graphql/lib/elastic_graph/graphql/resolvers/nested_relationships_source.rb index 944b13205..f5098739d 100644 --- a/elasticgraph-graphql/lib/elastic_graph/graphql/resolvers/nested_relationships_source.rb +++ b/elasticgraph-graphql/lib/elastic_graph/graphql/resolvers/nested_relationships_source.rb @@ -8,6 +8,10 @@ require "elastic_graph/graphql/resolvers/query_source" +# :nocov: +require "elastic_graph/graphql/jruby_patches" if RUBY_ENGINE == "jruby" +# :nocov: + module ElasticGraph class GraphQL module Resolvers diff --git a/elasticgraph-local/spec/acceptance/rake_tasks_spec.rb b/elasticgraph-local/spec/acceptance/rake_tasks_spec.rb index ec56ade44..539166148 100644 --- a/elasticgraph-local/spec/acceptance/rake_tasks_spec.rb +++ b/elasticgraph-local/spec/acceptance/rake_tasks_spec.rb @@ -20,7 +20,7 @@ module Local # config files being relative to the root. around { |ex| Dir.chdir(CommonSpecHelpers::REPO_ROOT, &ex) } - it "supports fully booting from scratch via a single `boot_locally` rake task" do + it "supports fully booting from scratch via a single `boot_locally` rake task", :except_jruby do rack_port = 9620 kill_daemon_after "rackup.pid" do |pid_file| halt_datastore_daemon_after :elasticsearch do @@ -56,7 +56,7 @@ module Local end context "when the datastore is not running" do - it "boots the datastore as a daemon to support booting GraphiQL" do + it "boots the datastore as a daemon to support booting GraphiQL", :except_jruby do halt_datastore_daemon_after :elasticsearch do |datastore_port| expect { kill_daemon_after("rackup.pid") do |pid_file| diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/api.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/api.rb index 9096dd935..9be4d045d 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/api.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/api.rb @@ -13,6 +13,10 @@ require "elastic_graph/schema_definition/results" require "elastic_graph/schema_definition/state" +# :nocov: -- only loaded on JRuby +require "elastic_graph/schema_definition/jruby_patches" if RUBY_ENGINE == "jruby" +# :nocov: + module ElasticGraph # The main entry point for schema definition from ElasticGraph applications. # diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/jruby_patches.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/jruby_patches.rb new file mode 100644 index 000000000..aaf6751ec --- /dev/null +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/jruby_patches.rb @@ -0,0 +1,59 @@ +# Copyright 2024 - 2026 Block, Inc. +# +# Use of this source code is governed by an MIT-style +# license that can be found in the LICENSE file or at +# https://opensource.org/licenses/MIT. +# +# frozen_string_literal: true + +# Central location for JRuby workarounds in the schema_definition gem. +# Each patch should reference the upstream fix and specify when it can be removed. + +module ElasticGraph + module SchemaDefinition + # @private + module JRubyPatches + # Bug: `Thread::Backtrace::Location#absolute_path` returns a relative path (same as `#path`) + # when the source file was loaded via `load` with a bare relative path (e.g. `load "schema.rb"`). + # On MRI, `absolute_path` correctly resolves to the full absolute path in this case. + # Workaround: override `absolute_path` to expand relative paths. + # Reported upstream: https://github.com/jruby/jruby/issues/9245 + # TODO: remove once JRuby fixes this upstream. + # @private + module BacktraceLocationAbsolutePathPatch + def absolute_path + result = super + return result if result.nil? || result.start_with?("/") + ::File.expand_path(result) + end + end + + ::Thread::Backtrace::Location.class_exec do + prepend BacktraceLocationAbsolutePathPatch + end + end + end +end + +require "elastic_graph/schema_definition/mixins/verifies_graphql_name" +require "elastic_graph/schema_definition/mixins/has_indices" + +# Bug: `def initialize(...)` + `super(...)` (or `super(*args, **kwargs)`) in a module +# prepended/included on a Struct subclass incorrectly warns about keyword arguments +# (3+ members) or crashes with ClassCastException (1 member). +# Workaround: use `ruby2_keywords` to avoid separate `**kwargs` forwarding. +# Reported upstream: https://github.com/jruby/jruby/issues/9242 +# TODO: remove once JRuby fixes this upstream. +ElasticGraph::SchemaDefinition::Mixins::VerifiesGraphQLName.class_exec do + ruby2_keywords def initialize(*args, &block) + super(*args, &block) + ::ElasticGraph::SchemaDefinition::Mixins::VerifiesGraphQLName.verify_name!(name) + end +end + +ElasticGraph::SchemaDefinition::Mixins::HasIndices.class_exec do + ruby2_keywords def initialize(*args, &block) + super(*args, &block) + initialize_has_indices { yield self } + end +end diff --git a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_indices.rb b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_indices.rb index c60cdb857..9aaabc935 100644 --- a/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_indices.rb +++ b/elasticgraph-schema_definition/lib/elastic_graph/schema_definition/mixins/has_indices.rb @@ -26,11 +26,7 @@ module HasIndices # @private def initialize(*args, **options) super(*args, **options) - @runtime_metadata_overrides = {} - @can_configure_index = true - resolve_fields_with :get_record_field_value - yield self - @can_configure_index = false + initialize_has_indices { yield self } end # Converts the current type from being an _embedded_ type (that is, a type that is embedded within another indexed type) to an @@ -261,6 +257,14 @@ def fields_with_sources private + def initialize_has_indices + @runtime_metadata_overrides = {} + @can_configure_index = true + resolve_fields_with :get_record_field_value + yield + @can_configure_index = false + end + def self_update_target return nil if abstract? || !indexed? diff --git a/elasticgraph-schema_definition/spec/integration/elastic_graph/schema_definition/rake_tasks_spec.rb b/elasticgraph-schema_definition/spec/integration/elastic_graph/schema_definition/rake_tasks_spec.rb index 7bbc4d386..4fd907111 100644 --- a/elasticgraph-schema_definition/spec/integration/elastic_graph/schema_definition/rake_tasks_spec.rb +++ b/elasticgraph-schema_definition/spec/integration/elastic_graph/schema_definition/rake_tasks_spec.rb @@ -16,6 +16,16 @@ module ElasticGraph module SchemaDefinition RSpec.describe RakeTasks, :rake_task do + # JRuby has a bug (https://github.com/jruby/jruby/issues/9242) that causes spurious + # "keyword arguments" warnings on stderr when `initialize(...)` + `super(...)` is used + # in a module prepended on a Struct subclass. Our jruby_patches.rb works around this, + # but a load-order regression can silently re-expose the warnings. This hook ensures + # we catch that. We use `to_stderr_from_any_process` (fd-level) rather than `to_stderr` + # ($stderr-level) because one test below nests `to_stderr_from_any_process` inside this. + around do |example| + expect { example.run }.not_to output(/./).to_stderr_from_any_process + end + describe "schema_artifacts:dump", :in_temp_dir do it "idempotently dumps all schema artifacts, and is able to check if they are current with `:check`" do write_elastic_graph_schema_def_code(json_schema_version: 1) diff --git a/elasticgraph-support/lib/elastic_graph/support/config.rb b/elasticgraph-support/lib/elastic_graph/support/config.rb index 4db03da6c..7d95ae5b2 100644 --- a/elasticgraph-support/lib/elastic_graph/support/config.rb +++ b/elasticgraph-support/lib/elastic_graph/support/config.rb @@ -12,6 +12,10 @@ require "elastic_graph/support/hash_util" require "json_schemer" +# :nocov: -- only loaded on JRuby +require "elastic_graph/support/jruby_patches" if RUBY_ENGINE == "jruby" +# :nocov: + module ElasticGraph module Support # Provides a standard way to define an ElasticGraph configuration class. diff --git a/elasticgraph-support/lib/elastic_graph/support/jruby_patches.rb b/elasticgraph-support/lib/elastic_graph/support/jruby_patches.rb new file mode 100644 index 000000000..ade942314 --- /dev/null +++ b/elasticgraph-support/lib/elastic_graph/support/jruby_patches.rb @@ -0,0 +1,69 @@ +# Copyright 2024 - 2026 Block, Inc. +# +# Use of this source code is governed by an MIT-style +# license that can be found in the LICENSE file or at +# https://opensource.org/licenses/MIT. +# +# frozen_string_literal: true + +# Central location for JRuby workarounds. +# Each patch should reference the upstream fix and specify when it can be removed. + +if ::Gem::Version.new(JRUBY_VERSION) < ::Gem::Version.new("10.0.4.0") + module ElasticGraph + module Support + # @private + module JRubyPatches + # Bug 1: `Data.new(*args, **kwargs)` with empty kwargs breaks positional-to-keyword + # argument conversion. We patch every `Data.define`-created class so that `*args` and + # `**kwargs` are never forwarded together. + # Note: uses `alias_method` instead of `prepend`+`super` because `Factory.prevent_non_factory_instantiation_of` + # calls `undef_method :new` on some Data classes, which blocks `super` from finding the original `.new`. + # Fixed upstream: https://github.com/jruby/jruby/pull/9215, + # https://github.com/jruby/jruby/pull/9225 + # TODO: remove once we no longer support JRuby < 10.0.4.0. + # @private + module DataDefineNewPatch + def define(*members, &block) + klass = super + klass.singleton_class.class_eval do + alias_method :__original_new, :new + def new(*args, **kwargs) + if kwargs.empty? + __original_new(*args) + elsif args.empty? + __original_new(**kwargs) + else + raise ::ArgumentError, "`Data.new` does not accept mixed positional and keyword arguments" + end + end + end + klass + end + end + + ::Data.singleton_class.prepend(DataDefineNewPatch) + end + end + end + + # Bug 2: When a module overrides `initialize` with `super(**reordered_hash)` inside a + # `Data.define` block and the class is subclassed, `to_h`/`deconstruct` return values by + # position rather than name. Accessors are correct, so we derive values from them instead. + # Reported upstream: https://github.com/jruby/jruby/issues/9241 + # TODO: remove once we no longer support JRuby < 10.0.4.0. + ::Data.class_exec do + def to_h(&block) + result = self.class.members.to_h { |m| [m, public_send(m)] } + block ? result.to_h(&block) : result + end + + def deconstruct + self.class.members.map { |m| public_send(m) } + end + + def deconstruct_keys(keys) + keys ? to_h.slice(*keys) : to_h + end + end +end diff --git a/elasticgraph-support/lib/elastic_graph/support/memoizable_data.rb b/elasticgraph-support/lib/elastic_graph/support/memoizable_data.rb index 25071709c..790c52578 100644 --- a/elasticgraph-support/lib/elastic_graph/support/memoizable_data.rb +++ b/elasticgraph-support/lib/elastic_graph/support/memoizable_data.rb @@ -9,6 +9,10 @@ require "delegate" require "stringio" +# :nocov: -- only loaded on JRuby +require "elastic_graph/support/jruby_patches" if RUBY_ENGINE == "jruby" +# :nocov: + module ElasticGraph module Support # `::Data.define` in Ruby 3.2+ is *very* handy for defining immutable value objects. However, one annoying diff --git a/elasticgraph-support/spec/unit/elastic_graph/support/untyped_encoder_spec.rb b/elasticgraph-support/spec/unit/elastic_graph/support/untyped_encoder_spec.rb index 3cf5001bb..71af9f528 100644 --- a/elasticgraph-support/spec/unit/elastic_graph/support/untyped_encoder_spec.rb +++ b/elasticgraph-support/spec/unit/elastic_graph/support/untyped_encoder_spec.rb @@ -60,7 +60,7 @@ module Support # JSON doesn't support keys that aren't strings, but JSON.generate converts keys to strings. expect(::JSON.generate(data)).to eq('{"a":1,"3":"b","2":"d"}') # 3 and "a" aren't comparable when sorting... - expect { data.keys.sort }.to raise_error(/comparison of String with (2|3) failed/) + expect { data.keys.sort }.to raise_error(/comparison of \w+ with \w+ failed/) # ...but encode is still able to sort them. # # Note: JSON objects don't support non-string keys, so we should never actually hit this case, diff --git a/script/ci_parts/run_specs_for_jruby b/script/ci_parts/run_specs_for_jruby new file mode 100755 index 000000000..6c546979e --- /dev/null +++ b/script/ci_parts/run_specs_for_jruby @@ -0,0 +1,21 @@ +#!/usr/bin/env bash + +source "script/ci_parts/setup_env" "test" $1 $2 + +# We don't want to bother checking coverage on JRuby, for a few reasons: +# - The Ruby coverage APIs don't fully work on JRuby. +# - It slows things down. +# - We've chosen to annotate files with `# :nocov:` from an MRI perspective. Lines of code which are run on MRI but not JRuby don't have `# :nocov: +unset COVERAGE + +# On CI, this is quite slow, and when we use the progress formatter the CI build can appear to get stuck +# because 20+ minutes may pass before the line of progress dots completes, and GitHub actions streams build +# output line-by-line. We use the documentation formatter here so that the output has many lines and there's +# more obviously ongoing progress on CI. +script/run_specs --format documentation + +halt_datastore_daemon + +for gem in $gems_to_build_with_datastore_halted; do + bundle exec rspec $gem/spec --backtrace --format progress +done diff --git a/spec_support/lib/elastic_graph/spec_support/enable_simplecov.rb b/spec_support/lib/elastic_graph/spec_support/enable_simplecov.rb index ed6f0077e..0d87095f3 100644 --- a/spec_support/lib/elastic_graph/spec_support/enable_simplecov.rb +++ b/spec_support/lib/elastic_graph/spec_support/enable_simplecov.rb @@ -109,6 +109,9 @@ def wait_for_other_processes # ignore them here. add_filter "lib/elastic_graph/version.rb" if defined?(::ElasticGraph::VERSION) + # Don't track coverage of JRuby patch files as we only enforce coverage on MRI.. + add_filter "jruby_patches" + formatter SimpleCov::Formatter::MultiFormatter.new([ SimpleCov::Formatter::HTMLFormatter, SimpleCov::Formatter::Console,