Skip to content
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

Access to cookie config with accessor method #5526

Merged
merged 2 commits into from
Nov 25, 2020

Conversation

dejpec
Copy link
Contributor

@dejpec dejpec commented Oct 28, 2020

With this minimal change one can provide different cookie configs for the same SessionHandler
without coping the whole method into the inherited class.

Signed-off-by: Dejan Pecar dejan.pecar@abacus.ch

With this minimal change one can provide different cookie configs for the same SessionHandler
without coping the whole method into the inherited class.

Signed-off-by: Dejan Pecar <dejan.pecar@abacus.ch>
@gregw gregw requested review from janbartel and gregw October 28, 2020 17:36
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

ummmmmm not exactly sure what you say you want this for is a good idea..... but hey people use jetty in all sorts of weird and wonderful ways.

This change is harmless enough and probably makes the code more robust for a number of different weird things that could be done to _cookieConfig, so LGTM.

@janbartel what do you think?

@joakime
Copy link
Contributor

joakime commented Oct 28, 2020

How would this be used?

Would you override getSessionCookieConfig() and produce something different?

@dejpec
Copy link
Contributor Author

dejpec commented Oct 28, 2020

exactly. i want to have 2 different cookie configs and decide dynamically which one is used in
a derivation of SessionHandler.
it already works in my derivation but i had (like i mentioned) to copy the whole getSessionCookie method
with exactly that change in it which is not good for maintainablility.

@joakime
Copy link
Contributor

joakime commented Nov 16, 2020

@janbartel bump, can you review this?

@janbartel
Copy link
Contributor

LGTM, but @dejpec can you fix the conflict in your branch so it will apply cleanly?

Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

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

Seems fine, but please resolve the conflict in SessionHandler in your branch so we can apply it cleanly.

Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

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

LGTM

@janbartel janbartel merged commit 22c6b8f into jetty:jetty-10.0.x Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants