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

Add exclusion argument ("exclude") capability to API #2897

Closed
1 task done
patphongs opened this issue Jan 31, 2018 · 6 comments
Closed
1 task done

Add exclusion argument ("exclude") capability to API #2897

patphongs opened this issue Jan 31, 2018 · 6 comments
Assignees
Milestone

Comments

@patphongs
Copy link
Member

patphongs commented Jan 31, 2018

The capability of adding an exclusion argument to an API call would help build on functionality in which we need to exclude specific data from the call. This is especially pertinent to the RFAIs committee filings issue, https://github.com/18F/fec-cms/issues/1663, which would allow the call to exclude specific RFAIs and segment them out to particular tables.

@LindsayYoung suggested that exclusions could be built within the views.py as an additional funcitonality. Code reference below:

https://github.com/18F/openFEC/blob/8e39a1f8e04915ade2e97898b96eda2ea686e906/webservices/common/views.py#L44

  • Manually test multiple endpoints for all filter options: filter_match, filter_multi, and filter_fulltext type filter options. Pay special attention to combo include/exclude for fulltext searches. Consider taking inventory of which endpoints use these 3 sets of filters
@LindsayYoung
Copy link
Contributor

Here is the sqlalchemy syntax, for conjunctions like "not"
http://docs.sqlalchemy.org/en/latest/core/tutorial.html#conjunctions

In this case we would want to extend the filter multi field to parse out arguments that started with "-" we would then want those arguments to be in the exclusion kwargs and the other args to be processed as normal.

This is a common convention and also how we build exclusion queries in the legal endpoints
https://www.fec.gov/data/legal/search/?search=-test

I think this will be relatively easy to add to the filter and filter multi fields, I know this is also a sought after feature for the filter text fields as well.

@ccostino
Copy link
Contributor

The code in the views is ultimately calling methods defined in filters.py, which is where such changes are additional support would likely have to go. The upside of this is we could leverage these enhancements across the board. :-)

Using the filter_multi method as an example, we can see that it ultimately makes use of a SQLAlchemy Column Property Comparator called in_. There is also not_in, which serve the purpose of excluding things.

I'm not sure whether or not we want to incorporate any exclude functionality in these methods directly or write separate exclude methods that we could chain just like the existing filters, but either option will likely get us the result we want! Now it's just a matter of figuring out how to denote when something should be excluded instead of filtered by, and @LindsayYoung's suggestion of adding a - in front of the column name seems most appropriate.

@ccostino
Copy link
Contributor

Thinking through this a bit more, I think we'd be okay modifying the existing methods and performing the checks for a presence of - on the column names in them.

@patphongs
Copy link
Member Author

@LindsayYoung Just to answer your question about the elastic search exclude argument does use a - in the query. So using that to indicate exclusion seem standard. Will that conflict with the - in the sort parameters though? cc @ccostino

@ccostino
Copy link
Contributor

ccostino commented Feb 1, 2018

😆 great minds think alike, I brought the same thing up with @LindsayYoung about the confusion, @patphongs. :-)

It is generally standard practice (and not just with Elasticsearch) to use - to denote exclusion, and we would be okay given that the filter parameters are entirely separate from the sort parameters; different code processes each of those separately. The sorting logic all takes place in sorting.py, entirely separate from filters.py.

My main concern was folks being confused looking at the URL as the - would mean different things in different contexts, but there's plenty of precedents in the wild for this behavior, so I think we're okay. :-)

@jwchumley jwchumley changed the title Add exclusion argument capability to API Add exclusion argument ("exclude") capability to API Feb 9, 2018
@lbeaufort lbeaufort added this to the Sprint 5.1 milestone Feb 13, 2018
@AmyKort
Copy link

AmyKort commented Feb 21, 2018

Merged, so closing. Thanks!

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

No branches or pull requests

5 participants