service: Support null (#79) by rettichschnidi · Pull Request #189 · SAP/python-pyodata

rettichschnidi · 2022-01-02T21:26:05Z

Until now, null values received from a OData server get substituted by a
default value.
Users of this library can not tell apart a value which coincidentally
equals the default, and a missing (null-ed) value received from the
server.

This commit allows the user to disable the substitution of null values by
default values, retrieving a None object instead.

Questions:

Is this an acceptable approach to implement this features?

~~In case the maintainers agree with the direction, I will add tests and documentation.~~ (done)

phanak-sap · 2022-01-02T21:57:33Z

I think it is worth doing if there is genuine need for this - but not like additional hardcoded parameter for client and service classes, but instead new, optional parameter for the Config, that is already consumed by them. Default should be false, correct, to retain current behavior.

python-pyodata/pyodata/v2/model.py

Lines 101 to 104 in 21a99b5

    
           def __init__(self, 
        
                        custom_error_policies=None, 
        
                        default_error_policy=None, 
        
                        xml_namespaces=None):

It is the generic issue of specific problem of "Date valid minimum value vs default value" discussed in #143

But additionally, there is another layer of if-else possibilities - it depends also if the service metadata defines the attribute to be Nullable or not - see #102. if the retain_null=True and the service somehow returns Null on property that has metadata Nullable = False - it should fail obviously as currently is, not pass None object back.

rettichschnidi · 2022-01-12T00:06:43Z

I think it is worth doing if there is genuine need for this

For my use case (copying a 3rd-party database accessible via OData v2 into a SQL database) it is. Also, I think users generally expect to know when a value is missing (null), instead of getting a default value substituted.

I think it is worth doing if there is genuine need for this - but not like additional hardcoded parameter for client and service classes, but instead new, optional parameter for the Config, that is already consumed by them. Default should be false, correct, to retain current behavior.

I changed the Client to take the (extend) Config object. However, the server does not consume a Config, but a lit of arguments instead. Changing this would break the API.

But additionally, there is another layer of if-else possibilities - it depends also if the service metadata defines the attribute to be Nullable or not - see #102. if the retain_null=True and the service somehow returns Null on property that has metadata Nullable = False - it should fail obviously as currently is, not pass None object back.

Fixed it in the latest push.

Keeping as draft as I am still fighting the linter and I am unsure about how to document this properly.

codecov-commenter · 2022-01-23T23:45:39Z

Codecov Report

Merging #189 (b83823e) into master (0599db2) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #189      +/-   ##
==========================================
+ Coverage   92.66%   92.77%   +0.11%     
==========================================
  Files           6        6              
  Lines        2768     2783      +15     
==========================================
+ Hits         2565     2582      +17     
+ Misses        203      201       -2

Impacted Files	Coverage Δ
pyodata/client.py	`100.00% <100.00%> (ø)`
pyodata/v2/model.py	`93.38% <100.00%> (+0.07%)`	⬆️
pyodata/v2/service.py	`90.88% <100.00%> (+0.22%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0599db2...b83823e. Read the comment docs.

filak-sap · 2022-01-24T05:49:02Z

+.. code-block:: python
+
+    count = getattr(northwind.entity_sets, 'Employees').get_entities().count().execute()
+    print(count)


The documentation looks good except I am not sure how does the code sample relates to it?

Not at all 😵‍💫 - this slipped in by accident and the documentation part is still WIP. Would like to give a "real-world" example using Northwind, just need to find some entity which actually returns a null property.

filak-sap · 2022-01-24T05:51:25Z

@rettichschnidi How does this change solve #79? Does the reporter need to configure the service instance with retain_null=True?

rettichschnidi · 2022-01-24T09:10:22Z

How does this change solve #79?

Empty dates will return Null/None, not the default value.

Does the reporter need to configure the service instance with retain_null=True?

This and checking if the (nullable) properties are None.

phanak-sap · 2022-01-26T22:12:39Z

    """OData service"""

-    def __init__(self, url, schema, connection):
+    def __init__(self, url, schema, connection, retain_null=False):


I do not particularly like this API. generic parameters, followed by boolean flag.. it is not much future-proof, when need arise for fifth or sixth configuration parameter in service.

I understand that this is getting quite entangled an messy, because Config class is in the model.py module and so far was not used by service.py at at. And it should in fact be in the root next to client or exceptions, definitely above pyodata/v2, simply because same config parameters should work IMHO in future v4 support as well. Perhaps such move will be in the 2.0.0 version, where backward compatibility can be broken..

But service.py is already at the moment depending on model.py:

python-pyodata/pyodata/v2/service.py

Line 20 in 0599db2

from . import model

So why not add the config=None to Service, instead of retain_null=False.. and in client.py:72 just pass the same value as in client.py:68

python-pyodata/pyodata/client.py

Line 68 in 0599db2

schema = pyodata.v2.model.MetadataBuilder(metadata, config=config).build()

filak-sap · 2022-01-27T14:45:58Z

Could you please rebase to the latest master (I don't like merge commits).

rettichschnidi · 2022-01-29T13:31:54Z

Rebased, using model.Config, improved commit messages and adjusted the documented example to #197.

phanak-sap · 2022-01-31T10:20:41Z

            # create service instance based on model we have
            logger.info('Creating OData Service (version: %d)', odata_version)
-            service = pyodata.v2.service.Service(url, schema, connection)
+            service = pyodata.v2.service.Service(url, schema, connection, config=config)


yeah, much better :)

phanak-sap

Looks good to me. Please resolve the conflict in changelog.md, so we have nice one commit as so far and we are ready to merge to master (and release 1.9.0)

Until now, null values received from OData servers get substituted by type specific default values. Users could not differentiate between those substitutes and actual values retrieved by the server, which might coincidentally equal the default ones inserted by pyodata. This commit allows the user to disable the substitution of null values, retrieving a None object instead. Example for Edm.Binary: | Server | pyodata old behavior | pyodata new (retain_null=True) | |--------+----------------------+--------------------------------| | 0 | 0 | 0 | | null | 0 | None |

phanak-sap added the enhancement New feature or request label Jan 2, 2022

rettichschnidi force-pushed the rs/upstream/support-null branch from 25073fa to 0d362a8 Compare January 12, 2022 00:03

rettichschnidi force-pushed the rs/upstream/support-null branch from 0d362a8 to 9e2eb92 Compare January 23, 2022 23:43

filak-sap reviewed Jan 24, 2022

View reviewed changes

rettichschnidi force-pushed the rs/upstream/support-null branch from 9e2eb92 to 551ea1f Compare January 26, 2022 00:55

rettichschnidi marked this pull request as ready for review January 26, 2022 00:58

rettichschnidi requested a review from filak-sap January 26, 2022 00:59

rettichschnidi force-pushed the rs/upstream/support-null branch from 551ea1f to b83823e Compare January 26, 2022 01:02

rettichschnidi mentioned this pull request Jan 26, 2022

differentiate empty date #79

Closed

phanak-sap reviewed Jan 26, 2022

View reviewed changes

phanak-sap added this to the 1.9.0 milestone Jan 27, 2022

rettichschnidi force-pushed the rs/upstream/support-null branch 3 times, most recently from d4dc0a9 to dfd4b4b Compare January 29, 2022 12:28

rettichschnidi requested a review from phanak-sap January 29, 2022 13:32

phanak-sap modified the milestones: 1.10.0, 1.9.0 Jan 31, 2022

phanak-sap reviewed Jan 31, 2022

View reviewed changes

phanak-sap approved these changes Jan 31, 2022

View reviewed changes

rettichschnidi force-pushed the rs/upstream/support-null branch from dfd4b4b to ff4d60b Compare January 31, 2022 12:18

phanak-sap merged commit 7627426 into SAP:master Jan 31, 2022

Conversation

rettichschnidi commented Jan 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phanak-sap commented Jan 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rettichschnidi commented Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jan 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

filak-sap Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

rettichschnidi Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

rettichschnidi Jan 26, 2022

Choose a reason for hiding this comment

Uh oh!

filak-sap commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rettichschnidi commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phanak-sap Jan 26, 2022

Choose a reason for hiding this comment

Uh oh!

filak-sap commented Jan 27, 2022

Uh oh!

rettichschnidi commented Jan 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phanak-sap Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

phanak-sap left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rettichschnidi commented Jan 2, 2022 •

edited

Loading

phanak-sap commented Jan 2, 2022 •

edited

Loading

rettichschnidi commented Jan 12, 2022 •

edited

Loading

codecov-commenter commented Jan 23, 2022 •

edited

Loading

filak-sap commented Jan 24, 2022 •

edited

Loading

rettichschnidi commented Jan 24, 2022 •

edited

Loading

rettichschnidi commented Jan 29, 2022 •

edited

Loading

phanak-sap left a comment •

edited

Loading