Skip to content

Compatibility with older servers that support #rewind. - #515

Closed
olleolleolle wants to merge 1 commit into
slack-ruby:masterfrom
olleolleolle:patch-1
Closed

Compatibility with older servers that support #rewind.#515
olleolleolle wants to merge 1 commit into
slack-ruby:masterfrom
olleolleolle:patch-1

Conversation

@olleolleolle

@olleolleolle olleolleolle commented Jul 3, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR is an experiment to make slack-ruby-client able to be used with Webrick via rackup gem.

The "rewind-read-rewind" used here:

slack-ruby-client/lib/slack/events/request.rb

Lines 39 to 47 in 9e6d117

def body
@body ||= begin
input = http_request.body
input.rewind
body = input.read
input.rewind
body
end
end

...is no longer supported.

This change is about making the code compatible with older servers that offered rewind.

@dangerpr-bot

dangerpr-bot commented Jul 3, 2024 •
edited
Loading

Copy link
Copy Markdown
1 Warning
⚠️ There're library changes, but not tests. That's OK as long as you're refactoring existing code.

Generated by 🚫 Danger

@coveralls

coveralls commented Jul 3, 2024 •
edited
Loading

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 9776811795

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 30 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.08%) to 89.161%
Files with Coverage Reduction New Missed Lines %
spec/slack/real_time/concurrency/clients/async_spec.rb 1 87.5%
spec/slack/real_time/concurrency/with_concurrency_spec.rb 1 80.0%
lib/slack/real_time/concurrency/async.rb 28 1.39%
Totals Coverage Status
Change from base Build 7789505062: -0.08%
Covered Lines: 5092
Relevant Lines: 5711
💛 - Coveralls

@coveralls

coveralls commented Jul 3, 2024 •
edited
Loading

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 9776811795

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 30 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.002%) to 89.239%
Files with Coverage Reduction New Missed Lines %
spec/slack/real_time/concurrency/clients/async_spec.rb 1 87.5%
spec/slack/real_time/concurrency/with_concurrency_spec.rb 1 80.0%
lib/slack/real_time/concurrency/async.rb 28 1.39%
Totals Coverage Status
Change from base Build 7789505062: 0.002%
Covered Lines: 5100
Relevant Lines: 5715
💛 - Coveralls

olleolleolle added a commit to olleolleolle/slack-ruby-client that referenced this pull request Jul 3, 2024
@olleolleolle
CHANGELOG: Mention slack-ruby#515
5313d69
@coveralls

coveralls commented Jul 3, 2024 •
edited
Loading

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 9777240612

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 28 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.08%) to 89.161%
Files with Coverage Reduction New Missed Lines %
lib/slack/real_time/concurrency/async.rb 28 1.39%
Totals Coverage Status
Change from base Build 7789505062: -0.08%
Covered Lines: 5092
Relevant Lines: 5711
💛 - Coveralls

@coveralls

coveralls commented Jul 3, 2024 •
edited
Loading

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 9777240612

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 28 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.002%) to 89.239%
Files with Coverage Reduction New Missed Lines %
lib/slack/real_time/concurrency/async.rb 28 1.39%
Totals Coverage Status
Change from base Build 7789505062: 0.002%
Covered Lines: 5100
Relevant Lines: 5715
💛 - Coveralls

olleolleolle added a commit to olleolleolle/slack-ruby-client that referenced this pull request Jul 3, 2024
@olleolleolle
CHANGELOG: Mention slack-ruby#515
9d9a79e
@coveralls

coveralls commented Jul 3, 2024 •
edited
Loading

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 9777379775

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.08%) to 89.161%
Totals Coverage Status
Change from base Build 7789505062: -0.08%
Covered Lines: 5092
Relevant Lines: 5711
💛 - Coveralls

@coveralls

coveralls commented Jul 3, 2024 •
edited
Loading

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 9777379775

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 89.239%
Totals Coverage Status
Change from base Build 7789505062: 0.002%
Covered Lines: 5100
Relevant Lines: 5715
💛 - Coveralls

olleolleolle added a commit to olleolleolle/slack-ruby-client that referenced this pull request Jul 3, 2024
@olleolleolle
CHANGELOG: Mention slack-ruby#515
ca340ed
@olleolleolle olleolleolle changed the title request.rb: Compatibility with rackup's WEBRick handler Compatibility with older servers that support #rewind. Jul 3, 2024
olleolleolle added a commit to olleolleolle/slack-ruby-client that referenced this pull request Jul 3, 2024
@olleolleolle
CHANGELOG: Mention slack-ruby#515
8501e7b
@coveralls

coveralls commented Jul 3, 2024 •
edited
Loading

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 9777483025

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.08%) to 89.161%
Totals Coverage Status
Change from base Build 7789505062: -0.08%
Covered Lines: 5092
Relevant Lines: 5711
💛 - Coveralls

@coveralls

coveralls commented Jul 3, 2024 •
edited
Loading

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 9777501826

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 28 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.08%) to 89.161%
Files with Coverage Reduction New Missed Lines %
lib/slack/real_time/concurrency/async.rb 28 1.39%
Totals Coverage Status
Change from base Build 7789505062: -0.08%
Covered Lines: 5092
Relevant Lines: 5711
💛 - Coveralls

1 similar comment
@coveralls

coveralls commented Jul 3, 2024

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 9777501826

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 28 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.08%) to 89.161%
Files with Coverage Reduction New Missed Lines %
lib/slack/real_time/concurrency/async.rb 28 1.39%
Totals Coverage Status
Change from base Build 7789505062: -0.08%
Covered Lines: 5092
Relevant Lines: 5711
💛 - Coveralls

@coveralls

coveralls commented Jul 3, 2024 •
edited
Loading

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 9777483025

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 89.239%
Totals Coverage Status
Change from base Build 7789505062: 0.002%
Covered Lines: 5100
Relevant Lines: 5715
💛 - Coveralls

1 similar comment
@coveralls

coveralls commented Jul 3, 2024

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 9777483025

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 89.239%
Totals Coverage Status
Change from base Build 7789505062: 0.002%
Covered Lines: 5100
Relevant Lines: 5715
💛 - Coveralls

@coveralls

coveralls commented Jul 3, 2024 •
edited
Loading

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 9777501826

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 28 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.002%) to 89.239%
Files with Coverage Reduction New Missed Lines %
lib/slack/real_time/concurrency/async.rb 28 1.39%
Totals Coverage Status
Change from base Build 7789505062: 0.002%
Covered Lines: 5100
Relevant Lines: 5715
💛 - Coveralls

@olleolleolle olleolleolle changed the title Compatibility with older servers that support #rewind. Compatibility with older servers that support #rewind. Jul 3, 2024
@coveralls

coveralls commented Jul 3, 2024 •
edited
Loading

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 9777677201

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 30 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.08%) to 89.161%
Files with Coverage Reduction New Missed Lines %
spec/slack/real_time/concurrency/clients/async_spec.rb 1 87.5%
spec/slack/real_time/concurrency/with_concurrency_spec.rb 1 80.0%
lib/slack/real_time/concurrency/async.rb 28 1.39%
Totals Coverage Status
Change from base Build 7789505062: -0.08%
Covered Lines: 5092
Relevant Lines: 5711
💛 - Coveralls

1 similar comment
@coveralls

coveralls commented Jul 3, 2024

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 9777677201

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 30 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.08%) to 89.161%
Files with Coverage Reduction New Missed Lines %
spec/slack/real_time/concurrency/clients/async_spec.rb 1 87.5%
spec/slack/real_time/concurrency/with_concurrency_spec.rb 1 80.0%
lib/slack/real_time/concurrency/async.rb 28 1.39%
Totals Coverage Status
Change from base Build 7789505062: -0.08%
Covered Lines: 5092
Relevant Lines: 5711
💛 - Coveralls

@coveralls

coveralls commented Jul 3, 2024 •
edited
Loading

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 9777677201

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 30 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.002%) to 89.239%
Files with Coverage Reduction New Missed Lines %
spec/slack/real_time/concurrency/clients/async_spec.rb 1 87.5%
spec/slack/real_time/concurrency/with_concurrency_spec.rb 1 80.0%
lib/slack/real_time/concurrency/async.rb 28 1.39%
Totals Coverage Status
Change from base Build 7789505062: 0.002%
Covered Lines: 5100
Relevant Lines: 5715
💛 - Coveralls

@dblock

dblock commented Jul 3, 2024

Copy link
Copy Markdown
Collaborator

If you don't .rewind you end up with incorrect data, don't you? I think we need an integration test for the various servers at the very least before changing this.

Coveralls is being annoying re-running on every commit... maybe you know how to turn this off?

@dblock dblock mentioned this pull request Oct 5, 2026
Merged
dblock added a commit to dblock/slack-ruby-client that referenced this pull request Oct 5, 2026
@dblock
Support non-rewindable Rack inputs for Events signature verification
a1ece3d 
Follow up on slack-ruby#515 with signature and WEBrick handler integration coverage for Rack 2 and Rack 3, plus middleware ordering documentation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
dblock added a commit to dblock/slack-ruby-client that referenced this pull request Oct 5, 2026
@dblock
Support non-rewindable Rack inputs for Events signature verification
5703680 
Follow up on slack-ruby#515 with signature and WEBrick handler integration coverage for Rack 2 and Rack 3, plus middleware ordering documentation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
dblock added a commit to dblock/slack-ruby-client that referenced this pull request Oct 5, 2026
@dblock
Support non-rewindable Rack inputs for Events signature verification
22e28bc 
Follow up on slack-ruby#515 with signature and WEBrick handler integration coverage for Rack 2 and Rack 3, plus middleware ordering documentation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
dblock added a commit to dblock/slack-ruby-client that referenced this pull request Oct 5, 2026
@dblock
Support non-rewindable Rack inputs for Events signature verification
05970c0 
Follow up on slack-ruby#515 with signature and WEBrick handler integration coverage for Rack 2 and Rack 3, plus middleware ordering documentation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
dblock added a commit that referenced this pull request Oct 5, 2026
@dblock
Support non-rewindable Rack inputs for Events signature verification
093baf4 
Follow up on #515 with signature and WEBrick handler integration coverage for Rack 2 and Rack 3, plus middleware ordering documentation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dblock

dblock commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

GitHub Copilot (AI), posting on behalf of @dblock.

Thank you for the original conditional-rewind fix! This has now been completed and merged in #607, with credit to @olleolleolle. The successor preserves rewindable-input behavior, supports non-rewindable Rack 3 streams, adds unit and WEBrick/Rack integration coverage, and documents middleware ordering and buffering with Rack::RewindableInput::Middleware.

Closing this original PR as superseded by the merged implementation.

@dblock dblock closed this Oct 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@olleolleolle @dangerpr-bot @coveralls @dblock