`AnalyticsEvent.endpoint` includes the HTTP request method #66

eastandwestwind · 2022-06-13T18:51:16Z

Purpose
Allows http method to be captured in analytics events

Closes #65

…sEvent prop

docker-compose.yml

PSalant726

This is a great start, but there are a few more changes we'll need on this PR:

Please update L60 of the Makefile:

fideslog/Makefile

Line 60 in 1aca996

@$(RUN_NO_DEPS) black --check fideslog/

such that it becomes:
```
    @$(RUN_NO_DEPS) black --check --exclude="sdk/python/_version\.py" fideslog/
```
This is because this will be the first PR opened since versioneer was added to the repo, and so the CI checks have never been run with those auto-generated files included. We need to ignore _version.py from this check, or it will always fail.

In sdk/python/event.py:

Please update the AnalyticsEvent constructor docstring to match the updated documentation in api/models/analytics_event.py:

fideslog/fideslog/sdk/python/event.py

Line 36 in 1aca996

    
                   :param endpoint: For events submitted as a result of making API server requests, the API endpoint path that was requested. If a fully-qualified URL is provided, only the URL path will be persisted.

Please update the validation code to match the new validation code in api/models/analytics_event.py:

fideslog/fideslog/sdk/python/event.py

Lines 71 to 74 in 1aca996

    
           self.endpoint = None 
        
           if endpoint is not None: 
        
               assert urlparse(endpoint).path != "", "endpoint must include a URL path" 
        
               self.endpoint = endpoint

In tests/api/test_models.py, please update the test_full_payload fixture:

fideslog/tests/api/test_models.py

Line 24 in 1aca996

"endpoint": "https://www.example.com/path/string",

.gitignore

docker-compose.yml

fideslog/api/models/analytics_event.py

…t issues

eastandwestwind · 2022-06-14T01:18:20Z

Updated based on your suggestions, thanks for the review, @PSalant726 !

Re: isort and pytest and black- I suspect something weird is going on with my editor, where I run auto-format for black and isort, and the changes do not appear in git diff. Similarly, running pytest fails locally bc the command-line is not picking up on recently updated code in intelliJ. I'll see if I can sort this out tomorrow

fideslog/sdk/python/__init__.py

PSalant726

Thanks for making all the static analysis related changes! Sorry you got stuck with those, that's my fault 😞

This is looking really good now - just some minor suggestions!

.gitignore

fideslog/api/models/analytics_event.py

fideslog/sdk/python/__init__.py

fideslog/sdk/python/event.py

PSalant726

LGTM! Just one UX question for you.

fideslog/api/models/analytics_event.py

fideslog/sdk/python/__init__.py

The only way to guarantee that we store just the URL path is to accept a full URL, validate it, and then truncate it to only the URL path. Also prevents the Python SDK from importing from the API server code.

By modifying the API endpoint URL within the `AnalyticsEvent` validation mechanism, we prevent continued validations of `AnalyticsEvent` instances from succeeding. In order to guarantee that API endpoint URLs only have their paths stored in the database, truncation must occur immediately prior to writing the new record.

eastandwestwind

@PSalant726 Just 1 blocking change request, otherwise looking good!

fideslog/api/database/data_access.py

fideslog/api/models/analytics_event.py

eastandwestwind

Thumbs up, tested manually full-stack and all looks good.

65-endpoint-method- requires http method as part of endpoint Analytic…

9b8e309

…sEvent prop

eastandwestwind requested a review from PSalant726 June 13, 2022 18:51

fix isort and format

1aca996

eastandwestwind commented Jun 13, 2022

View reviewed changes

docker-compose.yml Show resolved Hide resolved

PSalant726 assigned eastandwestwind Jun 13, 2022

PSalant726 added the enhancement New feature or request label Jun 13, 2022

PSalant726 suggested changes Jun 14, 2022

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

docker-compose.yml Show resolved Hide resolved

fideslog/api/models/analytics_event.py Outdated Show resolved Hide resolved

fideslog/api/models/analytics_event.py Outdated Show resolved Hide resolved

PSalant726 changed the title ~~65-endpoint-method- requires http method as part of endpoint AnalyticsEvent prop~~ AnalyticsEvent.endpoint includes the HTTP request method Jun 14, 2022

eastandwestwind added 3 commits June 13, 2022 20:45

adds additional validation to endpoint, updates test model, other lin…

4126e52

…t issues

transform http method to upper

8270477

updated format of allowed http methods

c297318

eastandwestwind mentioned this pull request Jun 14, 2022

559-endpoint-log-events - Adds middleware for calling analytics events for each endpoint ethyca/fidesops#622

Merged

10 tasks

eastandwestwind added 4 commits June 14, 2022 18:57

formatting

16d60cb

pylint ignore _version.py

fb77bd6

adds pylint ignore directly to _version.py

49be50c

ignore module has no attr

2fa1fc0

eastandwestwind commented Jun 15, 2022

View reviewed changes

fideslog/sdk/python/__init__.py Show resolved Hide resolved

PSalant726 suggested changes Jun 15, 2022

View reviewed changes

eastandwestwind added 2 commits June 15, 2022 14:38

adjust type ignore placement, refactor endpoint validation

952f151

moves type ignore comment, runs black in venv so it actually works

944975d

PSalant726 previously approved these changes Jun 15, 2022

View reviewed changes

fideslog/api/models/analytics_event.py Outdated Show resolved Hide resolved

fideslog/sdk/python/__init__.py Show resolved Hide resolved

PSalant726 added 2 commits June 16, 2022 10:54

Require a full API endpoint URL in AnalyticsEvents

fcbb6b4

The only way to guarantee that we store just the URL path is to accept a full URL, validate it, and then truncate it to only the URL path. Also prevents the Python SDK from importing from the API server code.

PSalant726 dismissed their stale review via bd00add June 16, 2022 18:27

eastandwestwind commented Jun 16, 2022

View reviewed changes

fideslog/api/database/data_access.py Show resolved Hide resolved

fideslog/api/database/data_access.py Show resolved Hide resolved

fideslog/api/models/analytics_event.py Show resolved Hide resolved

Prevent potential IndexErrors

476ed8f

eastandwestwind commented Jun 16, 2022

View reviewed changes

PSalant726 approved these changes Jun 16, 2022

View reviewed changes

PSalant726 merged commit 2e5f1ab into main Jun 16, 2022

PSalant726 deleted the 65-endpoint-method branch June 16, 2022 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`AnalyticsEvent.endpoint` includes the HTTP request method #66

`AnalyticsEvent.endpoint` includes the HTTP request method #66

eastandwestwind commented Jun 13, 2022 •

edited by PSalant726

Loading

PSalant726 left a comment

eastandwestwind commented Jun 14, 2022

PSalant726 left a comment

PSalant726 left a comment

eastandwestwind left a comment

eastandwestwind left a comment

	self.endpoint = None
	if endpoint is not None:
	assert urlparse(endpoint).path != "", "endpoint must include a URL path"
	self.endpoint = endpoint

AnalyticsEvent.endpoint includes the HTTP request method #66

AnalyticsEvent.endpoint includes the HTTP request method #66

Conversation

eastandwestwind commented Jun 13, 2022 • edited by PSalant726 Loading

PSalant726 left a comment

Choose a reason for hiding this comment

eastandwestwind commented Jun 14, 2022

PSalant726 left a comment

Choose a reason for hiding this comment

PSalant726 left a comment

Choose a reason for hiding this comment

eastandwestwind left a comment

Choose a reason for hiding this comment

eastandwestwind left a comment

Choose a reason for hiding this comment

`AnalyticsEvent.endpoint` includes the HTTP request method #66

`AnalyticsEvent.endpoint` includes the HTTP request method #66

eastandwestwind commented Jun 13, 2022 •

edited by PSalant726

Loading