API /send_encounters #34

DariuszAniszewski · 2020-04-02T21:25:21Z

Rozwiązuje #7

jakublipinski

Jedno pytanie. Poza tym super

functions/send_encounters/main.py

arsoba

Do not forget to update Swagger. :-)

arsoba · 2020-04-03T11:27:12Z

functions/send_encounters/README.md

@@ -0,0 +1,58 @@
+# Get status endpoint


I guess it is a copy-paste from another file. Could we fix it?

arsoba · 2020-04-03T11:32:02Z

functions/send_encounters/README.md

+device_name
+app_version
+lang
+encounters


Could it be better to list encounter fields with datatypes?

arsoba · 2020-04-03T11:42:01Z

functions/send_encounters/main.py

+
+def send_encounters(request):
+    if not request.is_json:
+        return jsonify(


It would be better to unify formatting. See this line and 28, for example.

arsoba · 2020-04-03T11:46:09Z

functions/send_encounters/main.py

+
+    request_data = request.get_json()
+
+    for key in ["user_id", "platform", "os_version", "device_type", "app_version", "lang", "encounters"]:


You have to be careful using user_id from the request. It would be difficult to generate the existing user_id, but it is really easy to leak this data.

arsoba · 2020-04-03T11:48:03Z

functions/send_encounters/main.py

+                }
+            ), 422
+
+    user_id = request_data["user_id"]


It is better to use constants for the names of all request fields.

arsoba · 2020-04-03T11:50:50Z

functions/send_encounters/main.py

+        ), 401
+    rows_to_insert = []
+    for encounter in encounters:
+        for key in ["encounter_date", "beacon_id", "signal_strength"]:


Could move all validation to functions. It is better to keep the function as clear as possible.

refaktoryzacja walidacji zapytania

DariuszAniszewski · 2020-04-04T10:13:22Z

@arsoba thanks for review, I guess all your remarks are reflected in current, not WIP version ;) PTAL

bartekn

(proszę o informację czy reviews z zewnątrz są mile widziane 😉 )

bartekn · 2020-04-04T12:35:24Z

functions/send_encounters/main.py

+def _get_user_entity(user_id: str) -> Optional[Entity]:
+    device_key = datastore_client.key(USERS_DATASTORE_KIND, f"{user_id}")
+    return datastore_client.get(key=device_key)


Ta i podobne funkcje są zduplikowane w innych plikach. Może warto przenieść je w jedno miejsce. Nie wiem na ile Google CF pozwala na takie coś. Podobnie ze stałymi (takie same w /get_status).

Nie jest to trywialne w kontekście CF, ale mamy to w planach ;)

bartekn · 2020-04-04T12:36:41Z

functions/send_encounters/main.py

+                }
+            )
+
+    for encounter in request_data["encounters"]:


W przypadku gdy lista jest pusta, funkcja nie powinna się zakończyć (z błędem lub bez)?

słuszna uwaga, dzięki 👍

poprawione w 359d153

bartekn · 2020-04-04T13:35:18Z

functions/send_encounters/main.py

+            )
+        )
+
+    errors = bq_client.insert_rows(table, rows_to_insert)


Na stronie Quotas and Limits można znaleźć informację o maksymalnym rozmiarze requestu HTTP w przypadku Streaming inserts (używane przez insert_rows):

HTTP request size limit: 10 MB (see Note)
Exceeding this value will cause invalid errors.
Note: Internally the request is translated from HTTP json into an internal data structure. The translated data structure has its own enforced size limit. It's hard to predict the size of the resulting internal data structure, but the chance of hitting the internal limit is very low if you keep your HTTP requests to 5 MB or less.

Oczywiście 5 MB to sporo i jest szansa że to nigdy się nie wydarzy (zależy jak działa aplikacja mobilna) ale myślę, że z uwagi na fakt, że te dane są niezwykle ważne i nie wiadomo kiedy i czy nastąpi retry warto rozważyć trzymanie się limitu 5 MB przy dodawaniu tych danych. W praktyce trzeba wywołać insert_rows kilka razy z mniejszymi paczkami.

Dzięki za info - czy mógłbyś proszę przekuć to w Issue? Zajmiemy się tym wkrótce.

DariuszAniszewski · 2020-04-05T11:08:05Z

@bartekn dzięki za review - jasne, że są mile widziane

jakublipinski

Wygląda bardzo dobrze. Dzięki @DariuszAniszewski

Flexicon · 2020-04-05T20:42:58Z

functions/send_encounters/main.py

+    except InvalidRequestException as e:
+        return jsonify(
+            e.response,
+            e.status


Shouldn't this be:

return jsonify(e.response), e.status

?

Yes! Thanks!

kstrz · 2020-04-06T08:34:27Z

functions/send_encounters/README.md

+
+[Swagger](https://swagger.io/) file is available [here](../../docs/swagger.yaml). 
+
+### Deployment


Można zaznaczyć, że to jest deployment do testów. Wdrażanie powinno odbywać się przez terraforma.

Pomysł OK, ale zrobiłbym to w osobnym PR - póki co jest spójne z innymi readme

kstrz · 2020-04-06T08:41:40Z

functions/send_encounters/main.py

+        if not request_data[key]:
+            raise InvalidRequestException(422, {"status": "failed", "message": f"empty field: {key}"})
+
+    for encounter in request_data["encounters"]:


skoro mamy już KEY_ENCOUNTERS to można go tu użyć

jasne, dzięki!

kstrz · 2020-04-06T08:50:36Z

functions/send_encounters/main.py

+        {
+            "user_id": user_id,
+            "upload_id": upload_id,
+            "date": datetime.utcnow(),


Taka data może zostać potraktowana jako czas lokalny podczas zapisywania do datastore. Może lepiej używać dat ze strefą czasową datetime.now(tz=pytz.utc)

Datastore przechowuje daty w UTC więc datetime.utcnow() jest poprawne.

Jest poprawne o ile funkcja nie działa w innej strefie czasowej gdzie datetime.utcnow() mogłoby zostać potraktowane jako czas lokalny z tej strefy. https://docs.python.org/3/library/datetime.html#datetime.datetime.utcnow

…a aktualnego czasu

API /send_encounters

f05c9a0

DariuszAniszewski changed the title ~~API /send_encounters~~ [WIP] API /send_encounters Apr 2, 2020

DariuszAniszewski mentioned this pull request Apr 2, 2020

Aktualizacja danych dotyczących API /send_encounters ProteGO-Safe/specs#16

Merged

DariuszAniszewski requested review from TobKed, jakublipinski and kstrz April 2, 2020 21:34

jakublipinski previously approved these changes Apr 3, 2020

View reviewed changes

functions/send_encounters/main.py Outdated Show resolved Hide resolved

arsoba reviewed Apr 3, 2020

View reviewed changes

Merge branch 'master' into send-encounters

7cab4fd

DariuszAniszewski dismissed jakublipinski’s stale review via 7cab4fd April 3, 2020 15:50

wydzielenie stałych dla kluczy

8899a45

refaktoryzacja walidacji zapytania

DariuszAniszewski mentioned this pull request Apr 3, 2020

Aktualizacja danych przechowywanych w Datastore ProteGO-Safe/specs#35

Merged

Flagi dotyczące przetworzenia/potwierdzenia zgłoszenia przez GIS

f7f00aa

DariuszAniszewski force-pushed the send-encounters branch from a594cf9 to f7f00aa Compare April 4, 2020 08:36

DariuszAniszewski added 2 commits April 4, 2020 11:34

aktualizacja swagger'a

eb89eab

poprawki dokumentacji

87af53f

DariuszAniszewski marked this pull request as ready for review April 4, 2020 10:12

DariuszAniszewski changed the title ~~[WIP] API /send_encounters~~ API /send_encounters Apr 4, 2020

DariuszAniszewski requested review from jakublipinski and removed request for TobKed April 4, 2020 10:15

bartekn reviewed Apr 4, 2020

View reviewed changes

weryfikacja, czy zapytanie nie zawiera pustych parametrów

359d153

jakublipinski previously approved these changes Apr 5, 2020

View reviewed changes

Merge branch 'master' into send-encounters

3477044

Flexicon reviewed Apr 5, 2020

View reviewed changes

zaaplikowany black + poprawka jsonify

20c09d9

DariuszAniszewski dismissed jakublipinski’s stale review via 20c09d9 April 5, 2020 21:12

kstrz requested changes Apr 6, 2020

View reviewed changes

użycie stałej KEY_ENCOUNTERS oraz ustawienie strefy utc dla pobierani…

e8d4942

…a aktualnego czasu

DariuszAniszewski mentioned this pull request Apr 6, 2020

[WIP] prototyp uwspólnienia kodu między funkcjami #64

Closed

kstrz approved these changes Apr 7, 2020

View reviewed changes

DariuszAniszewski merged commit 2594507 into master Apr 7, 2020

DariuszAniszewski deleted the send-encounters branch April 7, 2020 07:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API /send_encounters #34

API /send_encounters #34

DariuszAniszewski commented Apr 2, 2020 •

edited

Loading

jakublipinski left a comment

arsoba left a comment

arsoba Apr 3, 2020

arsoba Apr 3, 2020

arsoba Apr 3, 2020

arsoba Apr 3, 2020

arsoba Apr 3, 2020

arsoba Apr 3, 2020

DariuszAniszewski commented Apr 4, 2020

bartekn left a comment

bartekn Apr 4, 2020

DariuszAniszewski Apr 5, 2020

bartekn Apr 4, 2020

DariuszAniszewski Apr 5, 2020

DariuszAniszewski Apr 5, 2020

bartekn Apr 4, 2020

DariuszAniszewski Apr 5, 2020

DariuszAniszewski commented Apr 5, 2020

jakublipinski left a comment

Flexicon Apr 5, 2020

DariuszAniszewski Apr 5, 2020 •

edited

Loading

kstrz Apr 6, 2020

DariuszAniszewski Apr 6, 2020

kstrz Apr 6, 2020

DariuszAniszewski Apr 6, 2020

kstrz Apr 6, 2020

DariuszAniszewski Apr 6, 2020

bartekn Apr 6, 2020

kstrz Apr 7, 2020


		request_data = request.get_json()

		for key in ["user_id", "platform", "os_version", "device_type", "app_version", "lang", "encounters"]:


		[Swagger](https://swagger.io/) file is available [here](../../docs/swagger.yaml).

		### Deployment

API /send_encounters #34

API /send_encounters #34

Conversation

DariuszAniszewski commented Apr 2, 2020 • edited Loading

jakublipinski left a comment

Choose a reason for hiding this comment

arsoba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DariuszAniszewski commented Apr 4, 2020

bartekn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DariuszAniszewski commented Apr 5, 2020

jakublipinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DariuszAniszewski Apr 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DariuszAniszewski commented Apr 2, 2020 •

edited

Loading

DariuszAniszewski Apr 5, 2020 •

edited

Loading