A few improvements #111

bartonip · 2020-06-26T10:51:42Z

Added <, <=, >, >= operators for GetEntitySetFilter
Made headers attribute on ODataHttpRequest actually affect the headers
Allow PATCH or PUT to be specified for EntityModifyRequest

filak-sap · 2020-06-26T13:19:55Z

CHANGELOG.md

 ### Fixed
 - URL encode $filter contents - Barton Ip
+- Headers attribute on ODataHttpRequest - Barton Ip


I am sorry but I cannot see any code for this last changelog entry.

filak-sap · 2020-06-26T14:14:40Z

Well done! Thank you. I pushed these commits:

90016c2
a735037
5a1040a

bartonip · 2020-06-26T22:47:39Z

pyodata/v2/service.py

        self._connection = connection
        self._url = url
        self._handler = handler
-        self._headers = headers
+        self.headers = headers


The changes happen in the following lines

Do you really need to make the member public? If so, I would prefer a property.

Gotcha, this way we can also have a set_headers method that can append/update headers.

bartonip · 2020-06-26T22:47:45Z

pyodata/v2/service.py

@@ -284,7 +284,7 @@ def execute(self):
        # pylint: disable=assignment-from-none
        body = self.get_body()

-        headers = {} if self._headers is None else self._headers
+        headers = self.headers


Here we should have headers = dict(self.headers) to make sure we don't polute someones dict.

bartonip · 2020-06-26T22:47:50Z

pyodata/v2/service.py

@@ -351,7 +351,7 @@ def get_path(self):
        return self._entity_set_proxy.last_segment + self._entity_key.to_key_string()

    def get_headers(self):
-        return {'Accept': 'application/json'}
+        return {'Accept': 'application/json', **self.headers}


bartonip · 2020-06-26T22:47:54Z

pyodata/v2/service.py

@@ -448,7 +448,7 @@ def get_body(self):
        return json.dumps(self._get_body())

    def get_headers(self):
-        return {'Accept': 'application/json', 'Content-Type': 'application/json', 'X-Requested-With': 'X'}
+        return {'Accept': 'application/json', 'Content-Type': 'application/json', 'X-Requested-With': 'X', **self.headers}


bartonip · 2020-06-26T22:47:58Z

pyodata/v2/service.py

@@ -541,7 +546,7 @@ def get_body(self):
        return json.dumps(body)

    def get_headers(self):
-        return {'Accept': 'application/json', 'Content-Type': 'application/json'}
+        return {'Accept': 'application/json', 'Content-Type': 'application/json', **self.headers}


bartonip · 2020-06-26T22:48:06Z

pyodata/v2/service.py

@@ -639,6 +644,7 @@ def get_headers(self):

        return {
            'Accept': 'application/json',
+            **self.headers


bartonip · 2020-06-26T22:48:11Z

pyodata/v2/service.py

@@ -699,6 +705,7 @@ def get_method(self):
    def get_headers(self):
        return {
            'Accept': 'application/json',
+            **self.headers


bartonip · 2020-06-26T22:48:17Z

pyodata/v2/service.py

@@ -1433,7 +1452,7 @@ def get_boundary(self):

    def get_headers(self):
        # pylint: disable=no-self-use
-        return {'Content-Type': 'multipart/mixed;boundary={}'.format(self.get_boundary())}
+        return {'Content-Type': 'multipart/mixed;boundary={}'.format(self.get_boundary()), **self.headers}


filak-sap · 2020-06-26T23:47:41Z

@bartonip OMG, my apologies, I skipped that commit - I saw "M" at the commit summary, I my brain interpreted it as a merge commit.

filak-sap

Well, due to lack coffein or something else, I made the method get_headers public, but the goal was to create a method for ancestors of the class ODataHttpRequest to return static or customer headers and not the actual headers for HTTP request - my idea is that a child class should not be forced to full fill parents responsibility which is to use the headers given in the constructor or set via a property/member in HTTP request. (We might add add the prefix _ to the method name).

filak-sap · 2020-06-27T00:02:49Z

pyodata/v2/service.py

@@ -235,11 +235,11 @@ def __repr__(self):
 class ODataHttpRequest:
    """Deferred HTTP Request"""

-    def __init__(self, url, connection, handler, headers=None):
+    def __init__(self, url, connection, handler, headers={}):


Please, don't use objects for default values. The default value is instantiated only once. In this case, once someone populates headers, all other consecutive calls of ODataHttpRequests() will receive the hedaers directory populate.

Or was that your intention?

That was my intention for my use case, a bit hacky but works well for us without needing to set global params.

However, in retrospect you are correct we should not be forcing the headers on all descendant classes as this is definitely not expected behaviour.

I will revise this to use the dict type as the default value.

filak-sap · 2020-06-27T00:03:42Z

pyodata/v2/service.py

        self._connection = connection
        self._url = url
        self._handler = handler
-        self._headers = headers
+        self.headers = headers


Do you really need to make the member public? If so, I would prefer a property.

filak-sap · 2020-06-27T00:40:22Z

pyodata/v2/service.py

@@ -284,7 +284,7 @@ def execute(self):
        # pylint: disable=assignment-from-none
        body = self.get_body()

-        headers = {} if self._headers is None else self._headers
+        headers = self.headers


Here we should have headers = dict(self.headers) to make sure we don't polute someones dict.

filak-sap · 2020-06-27T00:43:43Z

pyodata/v2/service.py

@@ -284,7 +284,7 @@ def execute(self):
        # pylint: disable=assignment-from-none
        body = self.get_body()

-        headers = {} if self._headers is None else self._headers
+        headers = self.headers

        # pylint: disable=assignment-from-none
        extra_headers = self.get_headers()


If you keep the get_headers method as it is this patch, then we don't need the line 287 at all. But I would rather keep the line 287 and remove all occurences of slef.headers from the redefined get_headers. I don't think its a good idea to force descendant classes to use the headers class member to return the correct headers - this is up to the parent class.

…data

filak-sap · 2020-06-28T21:32:49Z

I pushed a slightly modified and squashed version:
2d7cd16

I hope you don't mind I kept you as author even thought I changed the patch - I prefer clean history where every single commit make sense.

bartonip added 5 commits June 26, 2020 04:49

Fixed filter not working for services that require URLEncoded filters

866db9f

Merge remote-tracking branch 'upstream/master'

474adfb

Added additional operators to GetEntitySetFilter

87cfac9

Made headers attribute actually affect the headers

06a52b8

Allow method to be specified in EntityModifyRequest

95ad17f

bartonip changed the title ~~Added additional operators to GetEntitySetFilter~~ A few improvements Jun 26, 2020

Update CHANGELOG.md

3f98d27

filak-sap reviewed Jun 26, 2020

View reviewed changes

filak-sap closed this Jun 26, 2020

bartonip commented Jun 26, 2020

View reviewed changes

filak-sap reopened this Jun 27, 2020

filak-sap requested changes Jun 27, 2020

View reviewed changes

bartonip added 2 commits June 28, 2020 03:01

Fixed custom headers implementation as per SAP#111

b6351d4

Merge branch 'master' of https://github.com/daughterlycare/python-pyo…

517a24d

…data

filak-sap closed this Jun 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A few improvements #111

A few improvements #111

bartonip commented Jun 26, 2020 •

edited

Loading

filak-sap Jun 26, 2020

filak-sap commented Jun 26, 2020

bartonip Jun 26, 2020

filak-sap Jun 27, 2020

bartonip Jun 27, 2020

bartonip Jun 26, 2020

filak-sap Jun 27, 2020

bartonip Jun 26, 2020

bartonip Jun 26, 2020

bartonip Jun 26, 2020

bartonip Jun 26, 2020

bartonip Jun 26, 2020

bartonip Jun 26, 2020

filak-sap commented Jun 26, 2020

filak-sap left a comment

filak-sap Jun 27, 2020

bartonip Jun 27, 2020

filak-sap Jun 27, 2020

filak-sap Jun 27, 2020

filak-sap Jun 27, 2020

filak-sap commented Jun 28, 2020

A few improvements #111

A few improvements #111

Conversation

bartonip commented Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

filak-sap commented Jun 26, 2020

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

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

Choose a reason for hiding this comment

filak-sap commented Jun 26, 2020

filak-sap 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

Choose a reason for hiding this comment

filak-sap commented Jun 28, 2020

bartonip commented Jun 26, 2020 •

edited

Loading