Add ConnectionConfig Filters #675
Conversation
…e) and whether the connection is disabled.
…ection_config_filtering # Conflicts: # CHANGELOG.md
…uery so FastAPI interprets as a query param and not a request body per FastAPI docs.
|
|
||
| if connection_type: | ||
| query = query.filter(ConnectionConfig.connection_type.in_(connection_type)) | ||
|
|
||
| if disabled is not None: | ||
| query = query.filter(ConnectionConfig.disabled == disabled) | ||
|
|
There was a problem hiding this comment.
Just connection_type and disabled were detailed in the ticket - is there anything else I should add while I'm here?
There was a problem hiding this comment.
It looks like there are 2 more filters that are needed for the frontend.
One is Testing Status. This would be filtering based on the last_test_succeeded field. I think it needs to be able to filter based on true, false, and null. null would be for connections that have never been tested.
The second is System Type. In the mockups it looks it's for filtering for all SAAS connections or all Database connections. I'm not sure if this is possible with the current data model. I think it would conflict with the connection_type filter you added because it would be a second filter based on the same field. Does that seem correct?
There was a problem hiding this comment.
Thanks @TheAndrewJackson - I can add both testing status and system type. it's fine if it's a second filter on the same field, just important to note that if the user selects "saas" and then filters for "postgres" then they'll get no results.
There was a problem hiding this comment.
Re: Testing status, let's make sure we're being careful about the "null" status. Usually a None would mean, "don't filter", and in this case, you're wanting it to mean "we never tested it". Maybe we do string filters here, "passed", "failed", "untested", and None, or similar.
There was a problem hiding this comment.
OK I added filtering on ?test_status=passed, ?test_status=failed, and?test_status=untested, and then there's
?system_type=saas and ?system_type=database.
If someone filters on conflicting fields, they'll get no results: ?system_type=saas&connection_type=postgres.
| @@ -86,9 +86,15 @@ def get_connections( | |||
| db: Session = Depends(deps.get_db), | |||
| params: Params = Depends(), | |||
| search: Optional[str] = None, | |||
| disabled: Optional[bool] = None, | |||
| connection_type: Optional[List[str]] = Query(default=None), # type:ignore | |||
There was a problem hiding this comment.
to search for multiple connection_types do: ?connection_type=postgres&connection_type=mongo.
I would have preferred this syntax: ?connection_type=postgres,mongo for an OR query, but I am using the syntax in the docs https://fastapi.tiangolo.com/it/tutorial/query-params-str-validations/#query-parameter-list-multiple-values which works well with the autogenerated API docs. I just think we need to make sure our meaning is clear in our own docs.
|
@ethyca/docs-authors some items added that describe filtering options on connection configs. |
| elif system_type == SystemType.database: | ||
| query = query.filter( | ||
| ConnectionConfig.connection_type.notin_( | ||
| [ConnectionType.saas, ConnectionType.manual] |
There was a problem hiding this comment.
Pointing out that there's really 3 system types right now, saas, database, and manual. I've only added system_type filtering for the first two, but I can change.
There was a problem hiding this comment.
@pattisdr Does that backend support manual connection types right now?
There was a problem hiding this comment.
yes it does but there's not an easy way to interact with them yet, it is a connection type that requires a user to go manually perform some action and upload data back to fidesops.
There was a problem hiding this comment.
I see. Maybe we should add it in in the backend just to get ahead of the future update? The frontend can always leave the filtering option out if it isn't supposed to handle it yet.
pattisdr
changed the title
| failed = "failed" | ||
| untested = "untested" | ||
|
|
||
| def str_to_bool(self) -> Optional[bool]: |
There was a problem hiding this comment.
I like this! Good idea
| elif system_type == SystemType.database: | ||
| query = query.filter( | ||
| ConnectionConfig.connection_type.notin_( | ||
| [ConnectionType.saas, ConnectionType.manual] |
There was a problem hiding this comment.
@pattisdr Does that backend support manual connection types right now?
TheAndrewJackson
deleted the
fidesops_670_add_connection_config_filtering
branch
* Add connectionconfig filtering on connection_type (can search multiple) and whether the connection is disabled. * Update changelog. * Ignore mypy error - we intentionally want the default value to be a Query so FastAPI interprets as a query param and not a request body per FastAPI docs. * Remove unused union. * Add connection config filtering on system_type and test_status. * No elif after return. * Add manual system type for connectionconfig filtering.
Purpose
Add ConnectionConfig filters to support the frontend datastore management landing page.
Changes
api/v1/connection/?disabled=trueorapi/v1/connection/?disabled=false?test_status=passed,?test_status=failed, and?test_status=untested?system_type=saasand?system_type=databaseChecklist
CHANGELOG.mdfileCHANGELOG.mdfile is being appended toUnreleasedsection in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.Run Unsafe PR Checkslabel has been applied, and checks have passed, if this PR touches any external servicesTicket
Fixes #670