Rl feature/support ignore request #131

braindeaf · 2021-10-11T18:10:55Z

Came across a requirement today to ignore_request? based on the actual request. Sometimes path alone is not enough. This allows testing of the Rack::Request.

# config/initializers/grover.rb
Grover.configure do |config|
  # assigning a Proc
  config.ignore_request = ->(req) do
    req.host == 'www.example.com'
  end
  # matches `www.example.com/foo/bar/123.png`

  config.ignore_request = ->(req) do
    req.has_header?('X-BLOCK')
  end
  # matches `HTTP Header X-BLOCK`
end

syntax changes Add to Unreleased CHANGELOG

…iosity-main

Co-authored-by: Andrew Bromwich <[email protected]>

…uest

abrom

Change looks good @braindeaf 👍

A few minor issues with the specs though. Let me know if you need any more info

abrom · 2021-10-12T06:21:21Z

spec/grover/middleware_spec.rb

+            response = get 'http://www.example.org/foobarbar', {}, { 'X-BLOCK' => '1' }
+            expect(response.content_type).to eq 'text/html'
+
+            response = get 'http://www.therealexample.org/foobarbar.pdf'


ideally this should match the request on L890 (aside from the "block" header) so as to minimise the things changing between success and failure.

OK, I think I got you.

blocking with host (html request) returns html
2.blocking with host (pdf request) returns html

not blocking with host (html request) returns html

not blocking with host (pdf request) returns pdf

abrom · 2021-10-12T06:22:32Z

spec/grover/middleware_spec.rb

+            response = get 'http://www.example.org/foobazbar.pdf', {}, { 'X-BLOCK' => '1' }
+            expect(response.content_type).to eq 'text/html'
+
+            response = get 'http://www.example.org/foobarbar', {}, { 'X-BLOCK' => '1' }


not sure what this is adding to the tests? Ideally this and the next test would both NOT have the extra header. And I would suggest that the URI requested be the same for all (except for the .pdf extension where appropriate!)

OK, I think I got you.

blocking with header (html request) returns html

blocking with header (pdf request) returns html

not blocking with header (html request) returns html

not blocking with header (pdf request) returns pdf

abrom · 2021-10-12T06:23:48Z

spec/grover/middleware_spec.rb

+            response = get 'http://www.example.org/foobarbar'
+            expect(response.content_type).to eq 'text/html'
+
+            response = get 'http://www.therealexample.org/foobarbar.pdf'


same applies to these tests. The first two are www.example.org, the second two should be something different (not just this one). And I would also suggest that the path (except the extension where appropriate) should be the same. ie foobazbar[.pdf]

OK, I think I got you.

blocking with host (html request) returns html

blocking with host (pdf request) returns html

not blocking with host (html request) returns html

not blocking with host (pdf request) returns pdf

abrom · 2021-10-12T06:26:07Z

README.md

@@ -270,8 +270,7 @@ N.B. by default PNG and JPEG are not modified in the middleware to prevent break
 To enable them, there are configuration options for each image type as well as an option to disable the PDF middleware
 (on by default).

-If either of the image handling middleware options are enabled, the [ignore_path](#ignore_path) should also
-be configured, otherwise assets are likely to be handled which would likely result in 404 responses.  
+If either of the image handling middleware options are enabled, the [ignore_path](#ignore_path) and/or [ignore_request](#ignore_request) should also be configured, otherwise assets are likely to be handled which would likely result in 404 responses.  


Can you please split these over multiple lines. Although it doesn't make a difference for the rendered markdown, it's easier to read in most editors if the line length is kept under 120.

Suggested change

If either of the image handling middleware options are enabled, the [ignore_path](#ignore_path) and/or [ignore_request](#ignore_request) should also be configured, otherwise assets are likely to be handled which would likely result in 404 responses.

If either of the image handling middleware options are enabled, the [ignore_path](#ignore_path) and/or

[ignore_request](#ignore_request) should also be configured, otherwise assets are likely to be handled

which would likely result in 404 responses.

abrom

Looks like your specs were still changing multiple things. You should always try to just change one thing and see that the change has the desired outcome

spec/grover/middleware_spec.rb

abrom

Thanks @braindeaf 👍

abrom · 2021-10-12T11:06:36Z

FYI it looks like the recent Let's Encrypt certificate expiry is affecting the build system. I've got another PR in the works that should fix that (#132)

braindeaf · 2021-10-12T11:10:27Z

Excellent, thank you :)

abrom · 2021-10-12T11:37:48Z

Released in v1.0.6

Rob Lacey and others added 17 commits June 19, 2020 17:02

ensure cookies from incoming request are passed to Grover via Middleware

43882af

remove specs for JPEG, PNG. Not sure this is the same case as PDF

5c90f35

rubocop

e399795

parse coookies with Rack::Utils

ff4b27a

allow class length of 140 lines

50f384a

syntax changes Add to Unreleased CHANGELOG

Merge branch 'main' of https://github.com/Studiosity/grover into Stud…

8e1e8ee

…iosity-main

Merge branch 'Studiosity-main' into main

5772154

unescaped cookies should be escaped before passing them to Grover

fe13055

merge oddity

2d8c5cf

processor spec passing encoded cookies to puppeteer

16e30fe

rubocop

9af0521

rubocop

d343111

Update spec/grover/processor_spec.rb

c60110a

Co-authored-by: Andrew Bromwich <[email protected]>

added fix to CHANGELOG

36ff117

Merge remote-tracking branch 'upstream/main'

bbbf86b

Add support for blocking processing by testing the incoming Rack::Req…

4e13cff

…uest

updated README, set PR in CHANGELOG

52a9f4c

abrom requested changes Oct 12, 2021

View reviewed changes

spec clarification, README tidy

24abc19

braindeaf requested a review from abrom October 12, 2021 08:53

abrom reviewed Oct 12, 2021

View reviewed changes

spec/grover/middleware_spec.rb Outdated Show resolved Hide resolved

spec/grover/middleware_spec.rb Outdated Show resolved Hide resolved

Apply suggestions from code review

a0f4932

abrom approved these changes Oct 12, 2021

View reviewed changes

abrom merged commit 1293a9c into Studiosity:main Oct 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rl feature/support ignore request #131

Rl feature/support ignore request #131

braindeaf commented Oct 11, 2021 •

edited

Loading

abrom left a comment

abrom Oct 12, 2021

braindeaf Oct 12, 2021

abrom Oct 12, 2021

braindeaf Oct 12, 2021 •

edited

Loading

abrom Oct 12, 2021

braindeaf Oct 12, 2021 •

edited

Loading

abrom Oct 12, 2021

braindeaf Oct 12, 2021

abrom left a comment

abrom left a comment

abrom commented Oct 12, 2021

braindeaf commented Oct 12, 2021

abrom commented Oct 12, 2021

Rl feature/support ignore request #131

Rl feature/support ignore request #131

Conversation

braindeaf commented Oct 11, 2021 • edited Loading

abrom left a comment

Choose a reason for hiding this comment

abrom Oct 12, 2021

Choose a reason for hiding this comment

braindeaf Oct 12, 2021

Choose a reason for hiding this comment

abrom Oct 12, 2021

Choose a reason for hiding this comment

braindeaf Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

abrom Oct 12, 2021

Choose a reason for hiding this comment

braindeaf Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

abrom Oct 12, 2021

Choose a reason for hiding this comment

braindeaf Oct 12, 2021

Choose a reason for hiding this comment

abrom left a comment

Choose a reason for hiding this comment

abrom left a comment

Choose a reason for hiding this comment

abrom commented Oct 12, 2021

braindeaf commented Oct 12, 2021

abrom commented Oct 12, 2021

braindeaf commented Oct 11, 2021 •

edited

Loading

braindeaf Oct 12, 2021 •

edited

Loading

braindeaf Oct 12, 2021 •

edited

Loading