Skip to content

Move from proxies to well-defined adapters#441

Open
fatkodima wants to merge 1 commit intorack:mainfrom
fatkodima:store-adapters
Open

Move from proxies to well-defined adapters#441
fatkodima wants to merge 1 commit intorack:mainfrom
fatkodima:store-adapters

Conversation

@fatkodima
Copy link
Contributor

@fatkodima fatkodima commented Oct 12, 2019

This is a work in progress (failing tests, missing new tests and documentation), but basically is ready for review to verify basic ideas.

To add new adapters, all is needed is inheriting from base class and implementing required methods:

module Rack
  class Attack
    class MyStoreAdapter < StoreAdapter
      ...

Closes #380

grzuy
grzuy reviewed Oct 23, 2019
View reviewed changes
Copy link
Collaborator

@grzuy grzuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this one!

lib/rack/attack/store_adapters/store_adapter.rb Outdated
class << self
def inherited(klass)
@adapters << klass
end
Copy link
Collaborator

@grzuy grzuy Oct 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Will just this inherited piece suffice to add support to "add your own cache backend store", by making proxies lookup dynamic instead of hard-coded?

If so, I'll prefer to have that as a small first step, and then re-consider the other changes.

Copy link
Contributor Author

@fatkodima fatkodima Oct 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

If so, I'll prefer to have that as a small first step, and then re-consider the other changes.

You mean extracting as a separate PR?

Copy link
Collaborator

@grzuy grzuy Oct 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Awesome.

If so, I'll prefer to have that as a small first step, and then re-consider the other changes.

You mean extracting as a separate PR?

Yes :-)

@fatkodima fatkodima mentioned this pull request Oct 24, 2019
Merged
@fatkodima fatkodima changed the title Move from proxies to well-defined adapters [skip ci] Move from proxies to well-defined adapters Oct 25, 2019
@fatkodima
Copy link
Contributor Author

fatkodima commented Oct 25, 2019

Updated this accordingly to master. Added tests. This pr became a little bold.
@grzuy Please, review.

@grzuy
Copy link
Collaborator

grzuy commented Jan 14, 2020

This can be considered backwards incompatible change, so it should be considered for v7.x.

@fatkodima
Copy link
Contributor Author

fatkodima commented Nov 17, 2020

Hey, @grzuy
Wdyt on moving this (and my other PRs) forward? Is it time for v7.x?

@ioquatix ioquatix deleted the branch rack:main February 1, 2022 20:05
@ioquatix ioquatix closed this Feb 1, 2022
@ioquatix ioquatix reopened this Feb 1, 2022
@ioquatix ioquatix changed the base branch from master to main February 1, 2022 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@grzuy grzuy grzuy left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Move from proxies to well-defined adapters

3 participants

@fatkodima @grzuy @ioquatix