Feature/replace feature - [merged] #102

Closed benoitdm-oslandia closed 2 years ago

In GitLab by @azarz on Sep 9, 2022, 10:09

_Merges feature/replacefeature -> develop

ref #23 et #21

In GitLab by @azarz on Sep 9, 2022, 16:01

added 1 commit

71017597 - Create all tests for replace

Compare with previous version

In GitLab by @azarz on Sep 9, 2022, 17:36

added 1 commit

756e3363 - Make REPLACE tests more OGC compliant

Compare with previous version

In GitLab by @azarz on Sep 9, 2022, 17:55

added 2 commits

7ff162de - Fix failure in replace tests
f5c84422 - REPLACE mock OK

Compare with previous version

In GitLab by @azarz on Sep 9, 2022, 21:59

Les tests ne passent pas à cause de leur ordre de lancement...

In GitLab by @azarz on Sep 9, 2022, 22:16

added 1 commit

18244ac4 - Add TestReplaceAfterAll to reset catalog state after perfoming replaces

Compare with previous version

In GitLab by @azarz on Sep 9, 2022, 22:48

added 1 commit

b51aedaf - Add db support for replace feature

Compare with previous version

In GitLab by @azarz on Sep 9, 2022, 22:52

added 1 commit

bd9ef1c1 - Forgot an = in db ReplaceTableFeature

Compare with previous version

In GitLab by @azarz on Sep 10, 2022, 24:36

added 2 commits

ad15212c - fix(replace-db): hack SQL query for correct return
a4245bca - (replace-db) fix test to take into account inconsitency

Compare with previous version

In GitLab by @azarz on Sep 10, 2022, 24:36

J'ai trouvé une incohérence entre la base de données de tests et le catalogue. Dans le catalogue, les ID des features sont des String, alors que dans la base de données de test, ce sont des SERIAL (int). Du coup les tests sont différents pour le catalogue ou pour la BDD, ce qui n'est pas normal je pense...

https://git.oslandia.net/Client-projects/geoplateforme-ign-pg-featureserv/-/blob/develop/internal/data/catalog.go#L114 https://git.oslandia.net/Client-projects/geoplateforme-ign-pg-featureserv/-/blob/develop/util/test_db.go#L55

https://git.oslandia.net/Client-projects/geoplateforme-ign-pg-featureserv/-/merge_requests/11/diffs?commit_id=a4245bcac627bff2d1a79f6cb61f5051a335cd07#3f82e2d4db76dbf6b4a5336696c1f108fc41216f_157_158

In GitLab by @azarz on Sep 10, 2022, 24:47

added 1 commit

d2516a81 - (replace-db) move back feat.properties.id to feat.id

Compare with previous version

In GitLab by @azarz on Sep 10, 2022, 24:51

marked this merge request as ready

In GitLab by @azarz on Sep 10, 2022, 01:16

added 1 commit

f26606f8 - Add TestGetCollectionReplaceSchema and TestGetCollectionUpdateSchema

Compare with previous version

si tu rebases tu verras qu'il y a une nouvelle variable featureId qui fait tout ca

Je ne me souviens plus de ce qu'on avait dit mais je ne vois pas d'obligation dans la spec de retourner du 200 + content. Si il n'y a pas d'obligation, je suggère qu'on se limite au 204.

Nos tests sont trop simples et ne permettent pas de tester tous les types possibles (bool, decimal, entier, geometrie, etc.).

Est ce que ca ne serait pas plus simple de récupérer l'objet en base directement après l'update ?

Unable to replace new feature in Collection new ?

            case "create", "replace":
                // The "replace" schema is identical to the "create" schema.
                // http://docs.ogc.org/DRAFTS/20-002.html#feature-geojson
                return writeCreateItemSchemaJSON(ctx, w, tbl)

bizarre que ca apparaisse dans tes commits. Ca aurait venir avec le rebase

pareil : "bizarre que ca apparaisse dans tes commits. Ca aurait venir avec le rebase"

pareil

ne fonctionnera plus dès qu'on va augmenter la taille de l'api

ajout inutile

pareil que plus haut: "bizarre que ca apparaisse dans tes commits. Ca aurait venir avec le rebase"

@azarz bien :thumbsup: , ca m'a fait voir qq points qu'on pourrait améliorer (comme les types des objets qu'on teste). Sinon je pense qu'il y a eu un mixe étrange avec des commits de la MR de @lowzonenose

In GitLab by @azarz on Sep 12, 2022, 10:53

Commented on internal/api/openapi.go line 527

Dans Zoom j'avais posé la question et on n'avait pas dit ça

In GitLab by @azarz on Sep 12, 2022, 10:57

Commented on internal/api/openapi.go line 527

D'autant plus que pour le UPDATE, c'est la même chose dans la doc de l'OGC, et pour l'instant seul le 200 était documenté. Il faudra être cohérent là dessus

In GitLab by @azarz on Sep 12, 2022, 11:00

Commented on internal/data/catalog_db.go line 370

C'est ce que je fais via le RETURNING, mais le problème c'est que le id est retourné dans la propriété properties de la feature.

J'ai essayé d'utiliser les fonctions pour récupérer un objet en base (TableFeature() notamment), mais elles prennent en argument un *QueryParams, auxquel on n'a pas accès dans la méthode ReplaceTableFeature()

De plus si on décide de renvoyer un 204 vide, cette partie du code n'aura plus beaucoup d'intérêt...

In GitLab by @azarz on Sep 12, 2022, 11:01

Commented on internal/api/api.go line 92

erreur de copier coller, je vais régler ça

In GitLab by @azarz on Sep 12, 2022, 11:02

added 1 commit

029fcdb0 - Apply 1 suggestion(s) to 1 file(s)

Compare with previous version

In GitLab by @azarz on Sep 12, 2022, 11:03

Commented on internal/service/handler.go line 554

pour les trucs du genre : quand j'ai fait un rebase il y a eu beaucoup de conflits avec les modifs de JP : git n'arrivait pas à bien faire le diff (comme lors de mon premier rebase). Je pense que ça vient de là

In GitLab by @azarz on Sep 12, 2022, 11:03

Commented on internal/service/handler_test.go line 490

changed this line in version 12 of the diff

In GitLab by @azarz on Sep 12, 2022, 11:03

added 1 commit

24eec165 - Apply 1 suggestion(s) to 1 file(s)

Compare with previous version

en effet, je n'ai été très clair et je n'ai pas réagi à ce qu'à fait @lowzonenose : je préférerais qu'on se cantonne au 204 sans retour de data supplémentaire (que je trouve inutile).

Si on reste sur du 204 donc en effet cette partie devient inutile

In GitLab by @azarz on Sep 12, 2022, 11:12

added 23 commits

d54b6433 - 1 commit from branch develop
46bd5564 - DRAFT: Add handler_put_test.go in order to test REPLACE functionality
af9bb5ac - DRAFT: draft TestReplaceFeature method
fd0316ae - Add openApi Put route for REPLACE
c9790d82 - DRAFT: Add replace methods to catalog and handler
90346f16 - DRAFT: Replace feature test pseudo code
44e206e6 - Add checkItemEquals method
c2189e50 - checkItem methods fix
30bc7c74 - New test methods in handler_put_test.go
3a1f0d1e - add replace and update schma types in handleCollectionSchemas
1b3a879e - Create all tests for replace
a7d46bb5 - Make REPLACE tests more OGC compliant
b50cf46d - Fix failure in replace tests
dfdad1d0 - REPLACE mock OK
6fcdc0d8 - Add TestReplaceAfterAll to reset catalog state after perfoming replaces
cf83a6c9 - Add db support for replace feature
3d802c00 - Forgot an = in db ReplaceTableFeature
c88ecf43 - fix(replace-db): hack SQL query for correct return
5f0e76df - (replace-db) fix test to take into account inconsitency
b86cc8d1 - (replace-db) move back feat.properties.id to feat.id
4c2a4d23 - Add TestGetCollectionReplaceSchema and TestGetCollectionUpdateSchema
a17725e6 - Apply 1 suggestion(s) to 1 file(s)
6ed17c14 - Apply 1 suggestion(s) to 1 file(s)