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

Oracle Provider #1329

Merged
merged 40 commits into from
Sep 27, 2023
Merged

Oracle Provider #1329

merged 40 commits into from
Sep 27, 2023

Conversation

totycro
Copy link
Contributor

@totycro totycro commented Aug 3, 2023

Overview

Oracle Provider for pygeoapi implemented by @xkosubek

The implementation is modeled after the PostgreSQL provider and thus should be fairly consistent with related code in pygeoapi.

It supports all CRUD operations.

The tests are executed via GitHub Actions using an oracle docker container.

There is some documentation, specifically also for the SqlManipulator, which is a mechanism for custom extensions that is necessary for a certain private use case, but is designed to be useable for different contexts.

(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

Copy link
Member

@tomkralidis tomkralidis left a comment

Choose a reason for hiding this comment

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

Excellent work here and super easy/pleasant to review. See minor change requests. Thanks!


Sometimes it is necessary to use the Oracle client for the connection. In this case init_oracle_client must be set to True.

The provider supports a SQL-Manipulator-Plugin class. With this, the SQL statement could be manipulated. This is
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this level of information (code blocks, etc.) can go in pygeoapi/provider/oracle.py and specify in the doc to consult the plugin itself? We should keep these sections of the docs more user focused.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about to link here to the test class? There is the example class, too.
Something like:

Sometimes it is necessary to use the Oracle client for the connection. In this case init_oracle_client must be set to True.

The provider supports a SQL-Manipulator-Plugin class. With this, the SQL statement could be manipulated. This is
useful e.g. for authorization at row level or manipulation of the explain plan with hints.
An example an more informations about that feature you can find in the test class in tests/test_oracle_provider.py.

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member

Choose a reason for hiding this comment

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

Remove this file (given we have an example in tests.

Copy link
Member

Choose a reason for hiding this comment

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

If the full configuration files are beneficial to keep, alternatively consider creating an oracle docker example.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, there is no benefit. Everything is in the documentation, too. I only forgot to delete this file...

Copy link
Member

Choose a reason for hiding this comment

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

Remove this file.

# OTHER DEALINGS IN THE SOFTWARE.
#
# =================================================================

Copy link
Member

Choose a reason for hiding this comment

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

Sort dependencies as standard library, external packages, local imports, alphabetically. So:

import importlib
import json
import logging
from typing import Optional

import oracledb

from pygeoapi.provider.base import (
    BaseProvider,
    ProviderConnectionError,
    ProviderItemNotFoundError,
    ProviderQueryError
)

if bbox:
bbox_dict = {"clause": "", "properties": {}}

sdo_mask = "mask={}".format(sdo_mask)
Copy link
Member

Choose a reason for hiding this comment

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

use f-string

where_dict["properties"].update(bbox_dict["properties"])

if where_conditions:
where_dict["clause"] = " WHERE {}".format(
Copy link
Member

Choose a reason for hiding this comment

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

use f-string

module_name, class_name = module_class_string.rsplit(".", 1)
module = importlib.import_module(module_name)
LOGGER.debug(
"reading class {} from module {}".format(class_name, module_name)
Copy link
Member

Choose a reason for hiding this comment

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

use f-string

if super_cls is not None:
assert issubclass(
cls, super_cls
), "class {} should inherit from {}".format(
Copy link
Member

Choose a reason for hiding this comment

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

use f-string

), "class {} should inherit from {}".format(
class_name, super_cls.__name__
)
LOGGER.debug("initialising {} with params {}".format(class_name, kwargs))
Copy link
Member

Choose a reason for hiding this comment

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

use f-string

@xkosubek
Copy link
Contributor

Thanks for the laud! I'll try to fix everything in the next days.

Copy link
Member

Choose a reason for hiding this comment

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

If the full configuration files are beneficial to keep, alternatively consider creating an oracle docker example.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the configuration is included in the test class.

@xkosubek
Copy link
Contributor

All change requests are now implemented and commited!

@tomkralidis tomkralidis merged commit ca7f8fc into geopython:master Sep 27, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants