add support for TinyDB feature provider #1724

tomkralidis · 2024-07-15T02:08:40Z

Overview

Add support for TinyDB as a feature provider.

Additionally updates CITE test setup and docs.

Related Issue / discussion

Fixes #1723

Additional information

Dependency policy (RFC2)

I have ensured that this PR meets RFC2 requirements

Updates to public demo

I have ensured that breaking changes to the pygeoapi master demo server have been addressed
- https://github.com/geopython/demo.pygeoapi.io/blob/master/services/pygeoapi_master/local.config.yml

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

tests/test_tinydb_provider.py

tomkralidis · 2024-07-15T02:35:02Z

FYI the associated CITE instance PR can be merged if/once this is approved/merge.

docs/source/data-publishing/ogcapi-features.rst

pygeoapi/plugin.py

justb4

Two main comments:

refactor Catalogue-specific statements into subclass TinyDBCatalogueProvider
add also unit tests for TinyDBCatalogueProvider

justb4 · 2024-07-15T10:08:39Z

pygeoapi/provider/tinydb_.py

There are quite some if self._catalogue statements in the base class TinyDBProvider, while at the same time there is a subclass TinyDBCatalogueProvider . My suggestion is to refactor those conditionals into the subclass.

That was my original implementation, but I decided to add this conditional (3x) in the code so as not to have to loop after search results which would affect performance.

But you may use overloaded functions and make _excluded a class var extended in. See rough example attached:

tinydb_.py.txt

(maybe not use return values, but like:

def _add_text_search_field(self, fields): fields['q'] = {'type': 'string'} // return fields

tomkralidis · 2024-07-15T21:03:49Z

Two main comments:

* refactor Catalogue-specific statements into subclass `TinyDBCatalogueProvider`

* add also unit tests for `TinyDBCatalogueProvider`

Note that these already exist in tests/test_tinydb_catalogue_provider.py.

tomkralidis · 2024-07-16T00:54:21Z

@justb4 PR updated, for review.

justb4

See inline comments: think all will be resolved by simply removing self.fields = self.get_fields() from constructor.

justb4 · 2024-07-16T09:32:11Z

pygeoapi/provider/tinydb_.py

+    def __init__(self, provider_def):
+        super().__init__(provider_def)
+
+        self._excludes = ['_metadata-anytext']


Major: IMO will not have desired effect: self.fields = self.get_fields() is already called in the base class constructor. Or self.fields = self.get_fields() should be called again, but see my suggestion to make _excludes a class var as it is in effect a property of the classes.

To consider: self.fields is never referenced after assignment so maybe self.fields = self.get_fields() can be removed and the above self._excludes = -statement will be ok. get_fields() is a Provider base.py method and the way to access fields I guess.

If we remove self.fields = self.get_fields() from the constructor then this will affect other parts of pygeoapi.api.itemtypes. Suggest we leave this as is in this PR and address in a subsequent PR later on.

Ok, then the only remaining is IMO to resolve the proper _excludes.

Do you agree that the current order will not work? Hence my suggestion for a class var (see my example) or another solution? i.e. get_fields() will not include self._excludes = ['_metadata-anytext'] as it is set after the parent class constructor. Reversing will also not work, as it will be overridden as self._excludes = [] in base class. (That is why I usually do not recommend too much initialization in a constructor, but explicit init/lifecycle methods.)

The default TinyDB provider as a result of this PR has an empty _excludes member. The TinyDBCatalogue provider sets _excludes to remove a given property that is a pygeoapi "extra".

fwiw other providers make the call to self.get_fields() at the end of the init method which will either fetch self.fields if set, or set self.fields.

esri.py

sqlite.py

Happy to establish a normative behavior between the two implementations. self.fields = self.get_fields() is present in

postgresql.py

elasticsearch_.py

Good points. Suggest we address this as part of another PR

justb4 · 2024-07-16T12:01:57Z

pygeoapi/provider/tinydb_.py

+    def __init__(self, provider_def):
+        super().__init__(provider_def)
+
+        self._excludes = ['_metadata-anytext']


Do you agree that the current order will not work? Hence my suggestion for a class var (see my example) or another solution? i.e. get_fields() will not include self._excludes = ['_metadata-anytext'] as it is set after the parent class constructor. Reversing will also not work, as it will be overridden as self._excludes = [] in base class. (That is why I usually do not recommend too much initialization in a constructor, but explicit init/lifecycle methods.)

tomkralidis · 2024-07-16T12:10:14Z

@justb4 this order works and accomplishes the need to remove the specified fields from catalogue responses.

justb4 · 2024-07-16T12:35:07Z

@justb4 this order works and accomplishes the need to remove the specified fields from catalogue responses.

Hmm, I still don't think so. Here is a similar example I used to investigate, showing both the instance and class var:

class Base:
    # class variable
    class_var = ['Base']
    def __init__(self):
        self.exclude = 'Exclude by Base'
        print('Parent init')
        self.show_info()
    def show_info(self):
        # Accessing class variable
        print("Base: self.exclude=", self.exclude)
        print("Base: Base.class_var=", Base.class_var)

class Derived(Base):

    def __init__(self):
        print('Derived init')
        Base.class_var.append('Derived')
        super().__init__()
        self.exclude = 'Exclude by Derived'

    def show_info(self):
        # Accessing class variable
        super().show_info()
        print("Derived: self.exclude=", self.exclude)
        print("Derived: Base.class_var=", Base.class_var)

derived = Derived()

self.exclude = 'Exclude by Derived' is called after the Base constructor. Guess the outcome...

tomkralidis · 2024-07-16T12:38:35Z

@justb4 feel free to test the branch with/without the exclude in the TinyDBCatalogue class?

justb4 · 2024-07-16T12:54:10Z

Ok, all tests pass. But makes sense as p.get_fields() is called. If I make

def test_query(config):
    p = TinyDBCatalogueProvider(config)

    fields = p.fields

as it results from constructor, the field is not excluded and test fails:

(pygeoapi-3.10.12) just@savu:~/project/pygeoapi/pygeoapi.git$ pytest tests/test_tinydb_catalogue_provider.py 
========================================================= test session starts =========================================================
platform darwin -- Python 3.10.12, pytest-8.2.2, pluggy-1.5.0
rootdir: /Users/just/project/pygeoapi/pygeoapi.git
configfile: pytest.ini
plugins: Faker-26.0.0, cov-5.0.0, env-1.1.3, anyio-4.4.0
collected 5 items                                                                                                                     

tests/test_tinydb_catalogue_provider.py F....                                                                                   [100%]

============================================================== FAILURES ===============================================================
_____________________________________________________________ test_query ______________________________________________________________

config = {'data': PosixPath('/private/var/folders/89/ggkklkwd5d7g9sw3pfbqwcvm0000gn/T/pytest-of-just/pytest-1/test_query0/sample-records.tinydb'), 'id_field': 'externalId', 'name': 'TinyDBCatalogue', 'time_field': 'created', ...}

    def test_query(config):
        p = TinyDBCatalogueProvider(config)
    
        fields = p.fields
>       assert len(fields) == 9
E       AssertionError: assert 10 == 9
E        +  where 10 = len({'_metadata-anytext': {'type': 'string'}, 'created': {'format': 'date-time', 'type': 'string'}, 'description': {'type': 'string'}, 'externalIds': {'type': 'string'}, ...})

tests/test_tinydb_catalogue_provider.py:97: AssertionError
======================================================= short test summary info =======================================================
FAILED tests/test_tinydb_catalogue_provider.py::test_query - AssertionError: assert 10 == 9
===================================================== 1 failed, 4 passed in 0.10s =====================================================
(py`

'_metadata-anytext' is not excluded... as can be expected.

justb4 · 2024-07-16T13:03:45Z

By using the class var, and an extra test, all is now well. Shall I commit/push my version on this branch?

tomkralidis · 2024-07-16T13:06:55Z

What if we normalized all access via get_fields(), and remove the .fields property? Again, this would be part of another PR.

justb4 · 2024-07-16T13:16:48Z

Function access is always to be preferred over exposing internal instance vars.

But harder to assess (performance) impact. As for some Providers it may mean accessing a remote source like a DB or even WFS. But then again get_fields() may smart-cache. Could even be implemented once in Base class e.g. with @property ...like inBaseProvider:

@property
def fields(self):
  if (not self._fields) {
    self._fields = self.get_fields()
  return self._fields

tomkralidis · 2024-07-16T13:24:20Z

\@Property
def fields(self):
return self.get_fields()

I guess you mean:

@property
def fields(self):
    return self.get_fields()

Suggest we address as part of another PR (given the scope of this PR is to expand TinyDB support). While we've identified some inconsistency/issue with .fields/get_fields(), this is better addressed in a separate PR given it will be a substantial change across numerous feature/record providers and their calling code.

@justb4 works for you?

justb4 · 2024-07-16T13:40:30Z

Ok, yes, but in my code I meant to cache the fields on first access (or leave that to each provider). Still in the current state TinyDBCatalogueProvider self.fields is incorrect. But don't know how much impact that has.

tomkralidis · 2024-07-16T13:42:07Z

ll in the current state TinyDBCatalogueProvider self.fields is incorrect. But don't know how much impact that has.

I think there is no impact in the context of the codepath and how the plugin is implemented.

justb4

Ok, we'll fix 'excluds' issue later. For sake of progress.

doublebyte1

Nice job!

tomkralidis added 4 commits July 14, 2024 22:03

add support for TinyDB as a feature provider (#1723)

975b1c6

add tests

23d8f4d

update CITE setup

2ce322d

add/update docs

c382997

tomkralidis added enhancement New feature or request OGC API - Features OGC API - Features labels Jul 15, 2024

tomkralidis added this to the 0.18.0 milestone Jul 15, 2024

update sample CITE config

516116f

tomkralidis changed the title ~~add support for TInyDB feature provider~~ add support for TinyDB feature provider Jul 15, 2024

tomkralidis requested review from pvgenuchten, totycro, kalxas, francbartoli, justb4, doublebyte1, jorgejesus, GeoSander, GregoryPetrochenkov-NOAA and webb-ben July 15, 2024 02:15

webb-ben approved these changes Jul 15, 2024

View reviewed changes

tests/test_tinydb_provider.py Outdated Show resolved Hide resolved

tomkralidis mentioned this pull request Jul 15, 2024

use TinyDB for OGC CITE feature test data geopython/demo.pygeoapi.io#58

Merged

tomkralidis added 2 commits July 14, 2024 22:36

update copyright year

27e6e5d

reorder imports

2b05302

justb4 reviewed Jul 15, 2024

View reviewed changes

docs/source/data-publishing/ogcapi-features.rst Outdated Show resolved Hide resolved

justb4 reviewed Jul 15, 2024

View reviewed changes

pygeoapi/plugin.py Show resolved Hide resolved

justb4 requested changes Jul 15, 2024

View reviewed changes

fix formatting

53409f7

delineate features/records via overloading

7bc816e

tomkralidis requested a review from justb4 July 16, 2024 00:54

tomkralidis added 4 commits July 15, 2024 20:59

remove now unused test data

3dc9828

move CITE data to tests/data

336c911

fix ref

1cebbec

remove HYDAT from GitHub Actions

26199cf

justb4 requested changes Jul 16, 2024

View reviewed changes

remove print statement

f3eaf5c

tomkralidis requested a review from justb4 July 16, 2024 10:12

justb4 requested changes Jul 16, 2024

View reviewed changes

webb-ben mentioned this pull request Jul 16, 2024

Use consistent get_field ref in providers #1727

Merged

5 tasks

tomkralidis requested a review from justb4 July 16, 2024 15:02

justb4 approved these changes Jul 16, 2024

View reviewed changes

tomkralidis merged commit d9adbbd into master Jul 16, 2024
9 checks passed

tomkralidis deleted the tinydb-features branch July 16, 2024 15:41

doublebyte1 reviewed Jul 16, 2024

View reviewed changes

tomkralidis mentioned this pull request Aug 13, 2024

update docstrings for base provider fields functionality #1783

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for TinyDB feature provider #1724

add support for TinyDB feature provider #1724

tomkralidis commented Jul 15, 2024

tomkralidis commented Jul 15, 2024

justb4 left a comment

justb4 Jul 15, 2024

tomkralidis Jul 15, 2024

justb4 Jul 15, 2024

justb4 Jul 15, 2024

tomkralidis commented Jul 15, 2024

tomkralidis commented Jul 16, 2024

justb4 left a comment

justb4 Jul 16, 2024

justb4 Jul 16, 2024

tomkralidis Jul 16, 2024

justb4 Jul 16, 2024

justb4 Jul 16, 2024 •

edited

Loading

tomkralidis Jul 16, 2024

webb-ben Jul 16, 2024 •

edited

Loading

tomkralidis Jul 16, 2024

justb4 Jul 16, 2024 •

edited

Loading

tomkralidis commented Jul 16, 2024

justb4 commented Jul 16, 2024

tomkralidis commented Jul 16, 2024

justb4 commented Jul 16, 2024

justb4 commented Jul 16, 2024

tomkralidis commented Jul 16, 2024

justb4 commented Jul 16, 2024 •

edited

Loading

tomkralidis commented Jul 16, 2024 •

edited

Loading

justb4 commented Jul 16, 2024

tomkralidis commented Jul 16, 2024

justb4 left a comment

doublebyte1 left a comment

add support for TinyDB feature provider #1724

add support for TinyDB feature provider #1724

Conversation

tomkralidis commented Jul 15, 2024

Overview

Related Issue / discussion

Additional information

Dependency policy (RFC2)

Updates to public demo

Contributions and licensing

tomkralidis commented Jul 15, 2024

justb4 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

tomkralidis commented Jul 15, 2024

tomkralidis commented Jul 16, 2024

justb4 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

justb4 Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webb-ben Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justb4 Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

tomkralidis commented Jul 16, 2024

justb4 commented Jul 16, 2024

tomkralidis commented Jul 16, 2024

justb4 commented Jul 16, 2024

justb4 commented Jul 16, 2024

tomkralidis commented Jul 16, 2024

justb4 commented Jul 16, 2024 • edited Loading

tomkralidis commented Jul 16, 2024 • edited Loading

justb4 commented Jul 16, 2024

tomkralidis commented Jul 16, 2024

justb4 left a comment

Choose a reason for hiding this comment

doublebyte1 left a comment

Choose a reason for hiding this comment

justb4 Jul 16, 2024 •

edited

Loading

webb-ben Jul 16, 2024 •

edited

Loading

justb4 Jul 16, 2024 •

edited

Loading

justb4 commented Jul 16, 2024 •

edited

Loading

tomkralidis commented Jul 16, 2024 •

edited

Loading