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

api: added preference param to eliminate ES bouncing issue #107

Merged
merged 1 commit into from
Oct 16, 2017

Conversation

ntarocco
Copy link
Contributor

@ntarocco ntarocco self-assigned this Oct 12, 2017
@ntarocco
Copy link
Contributor Author

@nharraud and @jbenito3 would you mind to have a quick look to these changes, after our discussions?
I still need to create the other PRs to fix the modules that are using this new feature.

address = request.headers.get('X-Forwarded-For', request.remote_addr)
if address is not None:
# An 'X-Forwarded-For' header includes a comma separated list of
# the
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the comment cut in a strange way?

@jbenito3
Copy link
Member

@ntarocco finally you cannot solve inside the init method? Do you need to spread the pain?

Copy link
Member

@nharraud nharraud left a comment

Choose a reason for hiding this comment

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

Looks good to me, just added one comment.

@ntarocco
Copy link
Contributor Author

@nharraud fixed thanks!

@ntarocco
Copy link
Contributor Author

@jbenito3 and for any other reviewer.

The only way I found (and deeply discusses with @nharraud) was to expose a factory instead of the class to add this preference extra param globally.
In the end, we thought that it is probably over killing to add it for any request, and we decided that who is using RecordsSearch can decide to use that param or not.

alg = hashlib.md5()
alg.update(user_hash.encode('utf8'))
return alg.hexdigest()
return None
Copy link
Member

Choose a reason for hiding this comment

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

IMHO we don't need return None is None by default :)

Copy link
Member

Choose a reason for hiding this comment

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

Saying it explicitly makes the code more readable. Otherwise it might look like a forgotten return.

Copy link
Member

@switowski switowski left a comment

Choose a reason for hiding this comment

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

Tests are failing on Python 3.5, @ntarocco can you please take a look? Ping me when they are fixed so I will merge.

@ntarocco
Copy link
Contributor Author

@switowski thanks, I was fixing it, now it should work!


Taken from Flask Login utils.py.
"""
address = request.headers.get('X-Forwarded-For', request.remote_addr)
Copy link
Member

Choose a reason for hiding this comment

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

You can just use request.remote_addr, since we expect that to be set correctly. In case of proxies, you need to use Werkzeug's WSGIProxyFix to set it correctly from X-Forwarded-For headers. In order to correctly determine the right user IP address, you need to know how many proxies are in-front of your webapp. For Invenio this is usually 2: Nginx + HAProxy, hence, making the users' IP appear as second in the list. :-)

@ntarocco ntarocco removed their assignment Oct 13, 2017
@ntarocco
Copy link
Contributor Author

@lnielsen I have fixed it as you proposed! Thanks for checking!

* added a method to RecordsSearch to return a new instance of Search
  with a new query param preference. The value of the param will not
  change for requests coming from the same user.

* closes inveniosoftware#106
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

search: should do the right thing to prevent bouncing results
6 participants