-
Notifications
You must be signed in to change notification settings - Fork 1
[NEP-20726] Dependency Pruning, Faraday 2.x and ruby compatibility #22
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: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| // For format details, see https://aka.ms/devcontainer.json. For config options, see the | ||
| // README at: https://github.com/devcontainers/templates/tree/main/src/ruby | ||
| { | ||
| "name": "happi_dev", | ||
| "image": "mcr.microsoft.com/devcontainers/ruby:1-3.4-bullseye", | ||
| // Features to add to the dev container. More info: https://containers.dev/features. | ||
| // "features": {}, | ||
|
|
||
| // Use 'forwardPorts' to make a list of ports inside the container available locally. | ||
| // "forwardPorts": [], | ||
|
|
||
| // Use 'postCreateCommand' to run commands after the container is created. | ||
| "postCreateCommand": "bundle install", | ||
|
|
||
| "mounts": [ | ||
| "source=happi-bundle,target=/usr/local/bundle,type=volume" | ||
| ] | ||
|
|
||
| // Configure tool-specific properties. | ||
| // "customizations": {}, | ||
|
|
||
| // Uncomment to connect as root instead. More info: https://aka.ms/dev-containers-non-root. | ||
| //"remoteUser": "root" | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| # Happi - AI Coding Instructions | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🙌 We might want to get these instructions in our codebases that use Happi, unless there's a way to include these in the copilot instructions of dependent codebases. |
||
|
|
||
| ## Project Overview | ||
| Happi is a lightweight Ruby gem that provides a pre-configured Faraday HTTP client wrapper for RESTful APIs. It assumes URL patterns like `https://hostname.com/api/v1/something` and handles OAuth2 authentication, multipart file uploads, and JSON/form-encoded requests. | ||
|
|
||
| ## Architecture & Design Patterns | ||
|
|
||
| ### Class-Based Configuration (Critical) | ||
| **Never use `Happi::Client` directly** - always create a derived class. Configuration is stored as class-level state, so using the base class directly causes cross-contamination between different API endpoints. | ||
|
|
||
| ```ruby | ||
| # CORRECT: Derive your own client | ||
| class MyClient < Happi::Client; end | ||
| MyClient.configure { |config| config.host = 'http://api.example.com' } | ||
|
|
||
| # WRONG: Never use base class directly | ||
| Happi::Client.configure { ... } # Will pollute all clients | ||
| ``` | ||
|
|
||
| This pattern is tested extensively in [spec/client_spec.rb](spec/client_spec.rb#L5-L85) which validates config isolation between base/derived classes at both class and instance levels. | ||
|
|
||
| ### Configuration Hierarchy | ||
| Config precedence: instance-level > class-level > defaults (from `Happi::Configuration.defaults`). | ||
|
|
||
| Instance config is set via initializer: `MyClient.new(oauth_token: 'abc123', timeout: 30)` | ||
|
|
||
| ## Key Components | ||
|
|
||
| - **[lib/happi/client.rb](lib/happi/client.rb)**: Main HTTP client with REST verbs (get/post/patch/delete), error handling, OAuth2 middleware integration | ||
| - **[lib/happi/configuration.rb](lib/happi/configuration.rb)**: Config object with defaults for host, port, timeout, version, use_json, log_level, token_type | ||
| - **[lib/happi/file.rb](lib/happi/file.rb)**: Handles file uploads with MIME type detection, converts to Faraday::UploadIO for multipart requests | ||
| - **[lib/happi/error.rb](lib/happi/error.rb)**: HTTP error hierarchy mapping status codes (400-504) to specific exception classes | ||
|
|
||
| ## Developer Workflows | ||
|
|
||
| ### Testing | ||
| ```bash | ||
| # Run all specs | ||
| bundle exec rspec | ||
|
|
||
| # Run specific spec file | ||
| bundle exec rspec spec/client_spec.rb | ||
| ``` | ||
|
|
||
| RSpec is configured with SimpleCov for coverage reports (see [coverage/index.html](coverage/index.html)). | ||
|
|
||
| ### Gemfile Variations | ||
| - `Gemfile`: Standard dependencies | ||
| - `Gemfile.rails32`, `Gemfile.rails41`: Legacy Rails compatibility testing (historical, not actively maintained) | ||
|
|
||
| ## Critical Conventions | ||
|
|
||
| ### Multipart File Handling | ||
| When a hash value responds to `#multipart`, it's automatically converted by `param_check` method. This enables seamless file uploads: | ||
|
|
||
| ```ruby | ||
| client.post('templates', template: { | ||
| name: 'test', | ||
| file: Happi::File.new('/path/to/file.docx') # Automatically becomes multipart | ||
| }) | ||
| ``` | ||
|
|
||
| ### JSON vs Form Encoding | ||
| Controlled by `config.use_json` flag: | ||
| - `false` (default): Uses multipart/form-encoded requests | ||
| - `true`: Encodes requests as JSON and parses JSON responses | ||
|
|
||
| ### OAuth2 Token Types | ||
| Set `token_type: 'bearer'` to pass OAuth tokens only as Authorization header (not as query param). This avoids faraday_middleware warnings. See [CHANGELOG.md](CHANGELOG.md#L30) for context. | ||
|
|
||
| ### Logging Behavior | ||
| - `log_level: :debug` logs full request bodies/params (can generate large logs) | ||
| - `log_level: :info` (default) logs only HTTP method and URL | ||
| - Rails logger auto-detected via `Rails.try(:logger)`, falls back to STDOUT | ||
|
|
||
| ### URL Construction | ||
| URLs automatically prefixed with `/api/#{version}/`. The `version` config defaults to `'v1'`. Example: | ||
| ```ruby | ||
| client.get('templates') # Requests /api/v1/templates | ||
| ``` | ||
|
|
||
| ## Dependencies | ||
| - **faraday ~> 2.13**: Core HTTP client | ||
| - **oauth2 ~> 2.0**: Authentication (via FaradayMiddleware::OAuth2) | ||
| - **activemodel >= 6.0**: Provides `with_indifferent_access` for response hashes | ||
| - **mime-types**: File MIME type detection | ||
| - Ruby >= 3.2.0 | ||
|
|
||
| ## Testing Patterns | ||
| Specs extensively test configuration isolation, multipart param checking, and connection options. When adding features: | ||
| 1. Test class-level vs instance-level config behavior | ||
| 2. Verify derived classes don't pollute base class state | ||
| 3. Use fixtures from [spec/fixtures/](spec/fixtures/) for file upload tests | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| name: Ruby | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❗ Are we intending to replace the buildkite build with this action? Looks like buildkite is still testing ruby 2.1.5 rbenv install -s 2.1.5
rbenv local 2.1.5Do we need to update or remove that?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I don't see buildkite configured. Happy to remove and stick with this action. We can switch to GH actions on small test suites (<3 minutes). |
||
|
|
||
| on: | ||
| pull_request: | ||
| push: | ||
| branches: [ "develop" ] | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| jobs: | ||
| spec: | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| ruby-version: ["3.2", "3.3", "3.4", "4.0"] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🌱 We could probably exclude 3.3 from testing as we're going from 3.2 to 3.4, and can likely assume if both of those build than so does 3.3. |
||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: ruby/setup-ruby@v1 | ||
| with: | ||
| ruby-version: ${{ matrix.ruby-version }} | ||
| bundler-cache: true # runs 'bundle install' and caches installed gems automatically | ||
| - name: RSpec | ||
| run: bundle exec rake spec | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 3.2.0 | ||
| 3.4.8 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❗ This Happi version is being referenced in https://github.com/rdytech/bdf-service/pull/857, which is bumping the BDF Service to ruby 4.0. Will this cause issues?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since happi is already passing on ruby 4 via actions and passing on BDF, no issues I can see. This version is just the default for who wants to work with happi locally. I didn't move to 4 because the devcontainer is still a little unstable. |
||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| # Faraday 2.x Compatibility Report for Happi Gem | ||
|
|
||
| ## Issue Validation ✅ RESOLVED | ||
|
|
||
| ### Test Results | ||
| Added compatibility tests to [spec/client_spec.rb](spec/client_spec.rb#L105-L130) that now pass: | ||
|
|
||
| ``` | ||
| 4 examples, 0 failures | ||
|
|
||
| Happi::Client | ||
| #connection | ||
| Faraday 2.x compatibility | ||
| creates a Faraday connection without FaradayMiddleware errors | ||
| returns a Faraday::Connection instance | ||
| properly sets OAuth2 Authorization header | ||
| with custom token_type | ||
| uses the specified token_type | ||
| ``` | ||
|
|
||
| All 41 tests in the full suite pass. | ||
|
|
||
| ### Root Cause | ||
| The gem currently uses `FaradayMiddleware::OAuth2` and `FaradayMiddleware::EncodeJson` on [lib/happi/client.rb](lib/happi/client.rb#L99-L103), but: | ||
|
|
||
| 1. **`faraday_middleware` gem is not in dependencies** - [happi.gemspec](happi.gemspec) only includes `oauth2 ~> 2.0` but not `faraday_middleware` | ||
| 2. **`faraday_middleware` is incompatible with Faraday 2.x** - As documented in [context/faraday_2.x.md](context/faraday_2.x.md), the `faraday_middleware` gem was NOT updated to support Faraday 2.0+ | ||
|
|
||
| ## Recommended Compatibility Adjustments | ||
|
|
||
| ### ✅ IMPLEMENTED: Replaced OAuth2 Middleware | ||
|
|
||
| Replaced `FaradayMiddleware::OAuth2` with manual Authorization header setting in [lib/happi/client.rb](lib/happi/client.rb#L98-L115): | ||
|
|
||
| ```ruby | ||
| def connection | ||
| @connection ||= Faraday.new(config.host) do |f| | ||
| # Set OAuth2 Authorization header | ||
| if config.oauth_token.present? | ||
| token_type = config.token_type.presence || 'Bearer' | ||
| f.headers['Authorization'] = "#{token_type} #{config.oauth_token}" | ||
| end | ||
|
|
||
| if self.config.use_json | ||
| f.request :json # Encodes request body as JSON | ||
| f.response :json # Parses JSON responses | ||
| else | ||
| f.request :multipart | ||
| f.request :url_encoded | ||
| end | ||
|
|
||
| f.adapter :net_http | ||
| end | ||
| end | ||
| ``` | ||
|
|
||
| ### ✅ IMPLEMENTED: Removed EncodeJson Middleware | ||
|
|
||
| Removed `FaradayMiddleware::EncodeJson` - Faraday 2.x has native JSON request/response handling via `f.request :json` and `f.response :json`. | ||
|
|
||
| ### ✅ IMPLEMENTED: Removed Custom JSON Middleware | ||
|
|
||
| Removed the undefined `JSON` constant middleware - Faraday's native JSON handling provides the same functionality. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 We're using similar middleware in JR. E.g. |
||
|
|
||
| ## Additional Recommendations | ||
|
|
||
| ### 5. Update Dependencies (Optional but Recommended) | ||
|
|
||
| Consider adding these modern Faraday middleware gems if you need their features: | ||
| ```ruby | ||
| # In happi.gemspec | ||
| spec.add_dependency 'faraday-retry', '~> 2.0' # For automatic retries | ||
| ``` | ||
|
|
||
| ### 6. Update Documentation | ||
|
|
||
| Update the following files to remove `FaradayMiddleware` references: | ||
| - [CHANGELOG.md](CHANGELOG.md#L30) - Update notes about `FaradayMiddleware::OAuth2` | ||
| - [.github/copilot-instructions.md](.github/copilot-instructions.md#L84) - Update dependency description | ||
|
|
||
| ### 7. Test Coverage | ||
|
|
||
| After implementing changes, ensure tests pass: | ||
| ```bash | ||
| bundle exec rspec spec/client_spec.rb:105 | ||
| bundle exec rspec # Run all tests | ||
| ``` | ||
|
|
||
| ## Migration Priority | ||
|
|
||
| 1. **HIGH**: Remove `FaradayMiddleware::OAuth2` usage (breaks all OAuth requests) | ||
| 2. **HIGH**: Remove `FaradayMiddleware::EncodeJson` usage (breaks JSON requests) | ||
| 3. **MEDIUM**: Remove/define custom `JSON` middleware | ||
| 4. **LOW**: Update documentation | ||
|
|
||
| ## Compatibility Matrix | ||
|
|
||
| | Component | Current | Status | Recommendation | | ||
| |-----------|---------|--------|----------------| | ||
| | Faraday | 2.13 | ✅ OK | Keep current | | ||
| | FaradayMiddleware | Not included | ❌ BROKEN | Remove usage | | ||
| | oauth2 gem | 2.0 | ✅ OK | Use directly | | ||
| | JSON encoding | Mixed | ⚠️ PARTIAL | Use Faraday native | | ||
|
|
||
| ## Next Steps | ||
|
|
||
| 1. ✅ Validation complete - tests added and failing as expected | ||
| 2. ✅ Implement OAuth2 header solution - Manual Authorization header implemented | ||
| 3. ✅ Remove `FaradayMiddleware::EncodeJson` line - Removed | ||
| 4. ✅ Address custom `JSON` middleware - Removed undefined constant | ||
| 5. ✅ Run tests to verify fixes - All 41 tests pass | ||
| 6. ⏳ Update documentation (optional) | ||
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.
🔥 Thanks for adding this