Pass in initialized schema as an option

[kbaum]

Not sure if this is a problem others have run into or if there is a better solution. We are using Graphlient within sidekiq and since it is a multithreaded process, we are creating an instance of Graphlient::Client during each job execution. Even though we are caching our schema on disk, our schema.json is large and there is significant time and memory spent loading the schema from disk. My thought is that we might be able to instead load one schema in a rails initializer and share it among all threads. Thus far it has seemed to work in a multithreaded sidekiq instance:

initializer code

$graphlient_schema = Graphlient::Schema.new('https://graphql.foo.com/graphql', 'lib/graphql_schema_foo.json')

Initialize graphlient client

Graphlient::Client.new(
  $graphlient_schema.http,
  schema_path: $graphlient_schema.path,
  schema: $graphlient_schema,
  headers: {
    'content-type' => 'application/json',
    'Authorization' => 'Bearer 123',
  }
)

Let me know what you think. Is it fully theadsafe to share a Graphlient::Schema like this?

[dblock]

Looks good! I don't see a reason why this can't work.

• I think with this schema_path and schema become mutually exclusive, so we should raise an error if both are specified?
• Add to README and CHANGELOG.

[kbaum]

I think with this schema_path and schema become mutually exclusive, so we should raise an error if both are specified?
Great point!

Will raise an error if both schema and schema_path are provided and add to README.

Thanks @dblock!

[kbaum]

@dblock raises an error from the constructor of both schema_path and schema are given as an option. Also added some documentation. Let me what you think.

Thanks!

[dblock]

I have some language nits, please?

[kbaum]

Addressed all comments. Have another look when you get a chance.

Thanks!

[dblock]

One more nit in the markdown and we're good to go. Consider the others. Thanks for hanging in here with me!

[kbaum]

Updated with the latest requested changes. Thanks for feedback!

[dblock]

Rubocop is failing, https://github.com/ashkan18/graphlient/actions/runs/6510603592/job/17703718899?pr=107, needs a rubocop -a ; rubocop --auto-gen-config.

Also I was thinking that this code looks a bit awkward, we pass both schema.http and schema.

client = Graphlient::Client.new(
  schema.http, # uses schema here
  schema: schema, # uses schema here
  headers: {
    'Authorization' => 'Bearer 123',
  }
)

Can we simplify further or rewrite this example not to use schema.http?

[kbaum]

Rubocop is failing, https://github.com/ashkan18/graphlient/actions/runs/6510603592/job/17703718899?pr=107, needs a rubocop -a ; rubocop --auto-gen-config.

Running into trouble on my local machine running rubocop. First trying with ruby-3.2.2 I get:

➜  graphlient git:(pass-in-loaded-schema) bundle exec rubocop
wrong number of arguments (given 5, expected 1)
/Users/karlbaum/.rbenv/versions/3.2.2/lib/ruby/3.2.0/psych.rb:322:in `safe_load'
/Users/karlbaum/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rubocop-0.56.0/lib/rubocop/config_loader.rb:185:in `yaml_safe_load'
/Users/karlbaum/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/rubocop-0.56.0/lib/rubocop/config_loader.rb:158:in `load_yaml_configuration'

Noticed the rubocop build uses ruby-2.7.2 so tried that and the funny thing is I get a message from rubocop saying this ruby version is not supported. I'm confused how it is working in the build.

➜  graphlient git:(pass-in-loaded-schema) bundle show rubocop
/Users/karlbaum/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/rubocop-0.56.0
➜  graphlient git:(pass-in-loaded-schema) bundle exec rubocop
Error: Unknown Ruby version 2.7 found in `.ruby-version`.
Supported versions: 2.1, 2.2, 2.3, 2.4, 2.5

Tried install ruby-2.5 since i had come this far but that gave me all sorts of errors including that it's not supported:

➜  graphlient git:(pass-in-loaded-schema) rbenv install 2.5.9
To follow progress, use 'tail -f /var/folders/bw/xp6pctg57gj8s_vxy0yyf53w0000gn/T/ruby-build.20231014123604.94280.log' or pass --verbose
Downloading openssl-1.1.1u.tar.gz...
-> https://dqw8nmjcqpjn7.cloudfront.net/e2f8d84b523eecd06c7be7626830370300fbcc15386bf5142d72758f6963ebc6
Installing openssl-1.1.1u...
Installed openssl-1.1.1u to /Users/karlbaum/.rbenv/versions/2.5.9

Downloading ruby-2.5.9.tar.bz2...
-> https://cache.ruby-lang.org/pub/ruby/2.5/ruby-2.5.9.tar.bz2
Installing ruby-2.5.9...

WARNING: ruby-2.5.9 is past its end of life and is now unsupported.
It no longer receives bug fixes or critical security updates.

ruby-build: using readline from homebrew
ruby-build: using libyaml from homebrew
ruby-build: using gmp from homebrew

BUILD FAILED (macOS 13.5.2 using ruby-build 20230717)

I finally got rubocop to somewhat work by changing back to ruby-2.7.2 and upgrading locally to rubocop 0.80.1. I had to also change TargetRubyVersion: 2.2 to TargetRubyVersion: 2.4 in .rubocop.yml. Even then, when I ran bundle exec rubocop, I got a bunch of cop failures not related to my changes.

Can't explain why it works in build, but maybe we should update rubocop to the latest version?

[kbaum]

Can we simplify further or rewrite this example not to use schema.http?

For now I took away the schema.http.

Thinking we could simplify further by allowing for passing in the schema as the first argument since the schema has a reference to the url.

client = Graphlient::Client.new(
  schema, # pass in url or schema here
  headers: {
    'Authorization' => 'Bearer 123',
  }
)

[dblock]

1 Warning
⚠️ Unless you're refactoring existing code or improving documentation, please update CHANGELOG.md.
Here's an example of a CHANGELOG.md entry:

* [#107](https://github.com/ashkan18/graphlient/pull/107): Pass in initialized schema as an option - [@kbaum](https://github.com/kbaum).

Generated by 🚫 Danger

You do need to add this too.

[dblock]

rubocop

I think you may see different results with doing bundle exec rubocop …, generally this gets sorted out when using rbenv or similar

Can't explain why it works in build, but maybe we should update rubocop to the latest version?

We should in a separate PR.

[kbaum]

Added changelog entry. Sorry I missed that. Thanks!

[dblock]

Merged, thanks!

[kbaum]

Thanks @dblock!