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

[WIP] OGC API Features - Part 4 / Support for PostgreSQLProvider #1266

Closed

Conversation

MTachon
Copy link
Contributor

@MTachon MTachon commented Jun 15, 2023

Overview

This PR is intended to add transactions support to the PostgreSQLProvider. Implementation should follow the OGC API - Features - Part 4: Create, Replace, Update and Delete draft specification.

Additional Information

The OGC API - Features - Part 4: Create, Replace, Update and Delete draft specification is not an OGC standard and is subject to change.

As of now, the draft specification does not mention the use of schemas for data validation in POST/PUT/PATCH (create/replace/update) requests. This is a work in progress and the schema related content of both Part 3 and Part 4 will likely be moved to a Part 5 specification document. One can get more information about this process at opengeospatial/ogcapi-features#740. Once more information about schemas is available and that this part of the specification is implemented in pygeoapi, the exception handling around the database transactions lines of code will be removed, and we will let schemas validation do the work and stop requests with invalid data.

Contributions and Licensing

(as per https://github.com/geopython/pygeoapi/blob/master/CONTRIBUTING.md#contributions-and-licensing)

  • I'd like to contribute [feature X|bugfix Y|docs|something else] to pygeoapi. I confirm that my contributions to pygeoapi will be compatible with the pygeoapi license guidelines at the time of contribution.
  • I have already previously agreed to the pygeoapi Contributions and Licensing Guidelines

The new function 'crs_transform_json_geom' used in the crs_transform decorator
allows the coordinates of a feature (GeoJSON-like dict) to be transformed
directly from one coordinate sytem to another, without first being converted to
a shapely geometrical object. This new pipeline: 'GeoJSON Feature ->
coordinates transformation' can replace the current pipeline: 'GeoJSON Feature
-> convertion to Shapely geometry -> coordinates transformation -> convertion
back to GeoJSON Feature'.
@MTachon MTachon marked this pull request as draft June 15, 2023 13:40
@MTachon
Copy link
Contributor Author

MTachon commented Jun 20, 2023

In the test_manage_collection_items_postgresql_create function, I create a new item in the database, and then I want to get back this newly created feature by using the feature's URI. I get a connection error (Failed to establish a new connection: [Errno 111] Connection refused) in the requests.get call, in the main.yml workflow. @tomkralidis , @justb4 , @webb-ben , @francbartoli , any ideas why?

session.commit()
session.refresh(db_obj_mapping)
identifier = getattr(db_obj_mapping, self.id_field)
except DataError as err:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @MTachon, why invalid data are reaching the database session? I'm expecting a data validation layer should be implemented at the interface level since pygeoapi already should be able to serve the schema for the collection. If the schema exposed is not validated than the endpoint should just emit a 422 response code with the validation error

Copy link
Contributor Author

@MTachon MTachon Jun 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @francbartoli, just checked for valid GeoJSON before that point. Was not sure if instantiating db_obj_mapping with wrong data passed to the table model would throw an error. No error came until the session.commit() statement. Not sure how it should be implemented. I also think it would be better with a data validation layer using schemas. As for now, I do not think pygeoapi serves collections' schemas, and in order to do so, we would need to implement a schema generation function for all providers. Right now, I am working on adding transactions support for PostgreSQLProvider, and I thought I could let PostgreSQL/PostGIS throw an error if the POST-ed data is invalid and catch the exception. ElasticsearchProvider supports transactions but is not using schemas validation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MTachon how does the specification handle the schema on the OpenAPI document? I'm expecting, as a developer, that the data model is exposed on the swagger page, is somehow the queryables endpoint designed to do that job?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this issue opengeospatial/ogcapi-features#424 the mutation schema is mentioned for that purpose, @tomkralidis any clue why it is completely missing from Part-4 now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@francbartoli , I think you will find your answer there: opengeospatial/ogcapi-features#740

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @MTachon, this is a WIP indeed. Can you please put a comment that this would be abstracted as soon as this part of the specification will be released and implemented in pygeoapi?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @francbartoli, just checked for valid GeoJSON before that point. Was not sure if instantiating db_obj_mapping with wrong data passed to the table model would throw an error. No error came until the session.commit() statement. Not sure how it should be implemented. I also think it would be better with a data validation layer using schemas. As for now, I do not think pygeoapi serves collections' schemas, and in order to do so, we would need to implement a schema generation function for all providers. Right now, I am working on adding transactions support for PostgreSQLProvider, and I thought I could let PostgreSQL/PostGIS throw an error if the POST-ed data is invalid and catch the exception. ElasticsearchProvider supports transactions but is not using schemas validation.

@MTachon FYI schema generation was implemented in #1022

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomkralidis , thanks for pointing it out! I was not aware of it, I will have a look.

@MTachon
Copy link
Contributor Author

MTachon commented Jun 20, 2023

In the test_manage_collection_items_postgresql_create function, I create a new item in the database, and then I want to get back this newly created feature by using the feature's URI. I get a connection error (Failed to establish a new connection: [Errno 111] Connection refused) in the requests.get call, in the main.yml workflow. @tomkralidis , @justb4 , @webb-ben , @francbartoli , any ideas why?

My mistake, no server is running here... Going to use methods from API instead, and extract the feature id from the returned URI in the Location header to send as parameter

@JakobMiksch
Copy link
Contributor

Thanks @MTachon for working on this.
Meanwhile QGIS 3.32.0 supports OGC API Features Part 4 as client. I just tried it with this PR, but it does not work yet (obviously, because it's still a draft :) )
I can offer to test more with QGIS and provide feedback. Just ping me if this is needed.

@MTachon
Copy link
Contributor Author

MTachon commented Aug 21, 2023

Hi @JakobMiksch . I have worked on other projects lately, so did not have much time to work on this PR. But now I will have more time again. I will let you know as I advance if some things can be tested :) Thanks!

@JakobMiksch
Copy link
Contributor

@MTachon alright, I am happy to test any time :)

@francbartoli
Copy link
Contributor

Hi @MTachon, what is the status of this?

@MTachon
Copy link
Contributor Author

MTachon commented Sep 17, 2023

Hi @francbartoli, I am not really satisfied by the start of my implementation of schemas... I would like like to use schema generation to filter out invalid data, and not do exception handling around the database session, as discussed above. I felt like schema generation was too much for the scope of this PR, and wanted to dedicate new PR for that. I started PR #1349 on GeoJSON schemas generation. It is an attempt to use pydantic models to provide a standard way for data validation and schema generation in the plugin providers implementation. I see that PR #1349 would required an upgrade of the pydantic version, but that should not be too much work, as addressed in issue #1341. I also get CI issues from type annotations and features not yet available in python 3.7. Upgrade of the python version in CI were discussed in issue #1176. If we do not want to upgrade the python version, I need to do some refactoring.

@MTachon MTachon mentioned this pull request Oct 4, 2023
2 tasks
Copy link

As per RFC4, this Pull Request has been inactive for 90 days. In order to manage maintenance burden, it will be automatically closed in 7 days.

@github-actions github-actions bot added the stale Issue marked stale by stale-bot label Mar 10, 2024
Copy link

As per RFC4, this Pull Request has been closed due to there being no activity for more than 90 days.

@github-actions github-actions bot closed this Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issue marked stale by stale-bot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants