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

Adding lazy service #67

Merged
merged 3 commits into from
Oct 30, 2013
Merged

Adding lazy service #67

merged 3 commits into from
Oct 30, 2013

Conversation

grandbora
Copy link
Contributor

This pr aims to fix #66. The problem is that FinatraServer assigns the value of service during construction, which doesn't leave any chance for the additional filters that are added later to be taken into account. Making this field lazy fixes the problem.

Testing this change without exposing the service turned out to be difficult. I tried my best but I am aware that the test is not the healthiest one.

@grandbora
Copy link
Contributor Author

@jkischkel please have look.

@jkischkel
Copy link

@grandbora looks good as a workaround, but if serivce gets accessed before we register custom filters, this will most likely still fail.

@grandbora
Copy link
Contributor Author

@twoism I actually wanted to write the test this way but the SpecHelper does not take the filters into account and furthermore the appService is not exposed therefore I could not find an easy way to test the filters as you described. However if I can refactor the SpecHelper then I can write a better test. I will try to do it soon-ish.

@twoism
Copy link
Contributor

twoism commented Oct 29, 2013

I was just looking at this as well. We should definitely have a filters spec so refactoring of the SpecHelper may need to happen. thanks for catching this!

@grandbora
Copy link
Contributor Author

@twoism I did a small refactor on SpecHelper and rewritten the test for the filter.

@twoism
Copy link
Contributor

twoism commented Oct 30, 2013

LGTM. Thanks!

twoism added a commit that referenced this pull request Oct 30, 2013
@twoism twoism merged commit 17ca180 into twitter:master Oct 30, 2013
@grandbora
Copy link
Contributor Author

@twoism thanks a lot for merging this. That saves us from a really ugly workaround.

@grandbora
Copy link
Contributor Author

Is it possible to have a (hotfix) release for this?

@capotej
Copy link
Contributor

capotej commented Nov 13, 2013

Yep, planning on doing a release tonight.

@grandbora
Copy link
Contributor Author

👍 Thanks.

@capotej
Copy link
Contributor

capotej commented Nov 13, 2013

1.4.1 should be out on sonatype with these fixes: https://github.com/capotej/finatra/releases/tag/finatra-1.4.1 Thanks!

cacoco pushed a commit that referenced this pull request May 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants