SAP / python-pyodata

Enterprise-ready Python OData client
Apache License 2.0
218 stars 93 forks source link

Get a count of entities with inlinecount #120

Closed s7oev closed 4 years ago

s7oev commented 4 years ago

Hello, I couldn't find any mention of "inlinecount" in the code.

The description of inlinecount can be seen in the URI conventions of OData 2, section 4.9.

Is this feature not supported in pyodata?

filak-sap commented 4 years ago

Hi @s7oev , than you for taking the time to report this issue. It's not supported yet but feel free to add it.

s7oev commented 4 years ago

@filak-sap thanks for your quick reply!

I'd love to try and contribute, although I can't promise I'll be able to deliver a working functionality.

Having said that, I actually managed to implement a basic inlinecount functionality already.

My only problem is how to effectively return this to the user. I.e., it would have to happen in the get_entities_handler() method, I believe. Here is it's current implementation:

 def get_entities_handler(response):
            """Gets entity set from HTTP Response"""

            if response.status_code != HTTP_CODE_OK:
                raise HttpError('HTTP GET for Entity Set {0} failed with status code {1}'
                                .format(self._name, response.status_code), response)

            content = response.json()

            if isinstance(content, int):
                return content

            entities = content['d']
            if isinstance(entities, dict):
                entities = entities['results']

            self._logger.info('Fetched %d entities', len(entities))

            result = []
            for props in entities:
                entity = EntityProxy(self._service, self._entity_set, self._entity_set.entity_type, props)
                result.append(entity)

            return result

The problem is that currently this method returns only a list of the entities. My very initial solution where the goal was only to see if I get the inlinecount successfully if it is requested was to add a new entry to the result as follows:

[...]
            entities = content['d']
            if isinstance(entities, dict):
                if '__count' in entities:
                    inlinecount = entities['__count']
[...]
            if inlinecount is not None:
                result.append({'Inlinecount': inlinecount})

Of course this is a very bad solution, as it mixes a list of entities with the inlinecount. I was thinking about making the result a dictionary if the inlinecount is requested, with keys e.g. result['inlinecount'] and result['entities'] but this is also a bad solution I feel because the function will return different types in different cases.

I'll continue thinking about the problem tomorrow, but was wondering if you (or anyone else) has some input with regards to this.

filak-sap commented 4 years ago

I don't think the later idea is that bad. I would create a wrapper class for the get entities results which implements interface of Python List and has the property total_count. The property total_count should raise an error if the request was made without $inlinecount=allpages. The user should not need to know that the property total_count is populated via the parameter $inlinecount=allpages as that's only V2 feature and V4 uses $count=true[0].

What about:

entities = theservice.EntitySet.get_entities().count(inline=True).execute()
print(entities.total_count)

matching_count = theservice.EntitySet.get_entities().count().execute()
print(matching_count)

entities_without_count = theservice.EntitySet.get_entities().execute()
try:
   print(entities.total_count)
except PyOData.exceptions.ProgramError as ex:
  print(str(ex))
# The collection does not include Total Count of items because the request was made without specifying `count(inline=True)`.

This way, users would not notice the difference between V2 and V4.

0: http://docs.oasis-open.org/odata/odata/v4.0/errata02/os/complete/part1-protocol/odata-v4.0-errata02-os-part1-protocol-complete.html#_Toc406398308

s7oev commented 4 years ago

@filak-sap I really liked the idea to implement List! Overall, the interface you suggest is very logical, especially combining the count function by setting up a flag of whether the result should be received inline (I had a separate inlinecount() function initially).

I already have the functionality working as in your example. Here's everything added to the code: Class QueryRequest, init function:

    def __init__(self, url, connection, handler, last_segment):
        [...]
        self._inlinecount = None
        [...]

Class QueryRequest, count function:

    def count(self, inline=False):
        """Sets a flag to return the number of items. Can be inline with results or just the count."""
        if inline:
            self._inlinecount = True
        else:
            self._count = True
        return self

Class QueryRequest, get_query_params function:

    def get_query_params(self):
        [...]
        if self._inlinecount:
            qparams['$inlinecount'] = 'allpages'
        [...]

Class EntitySetProxy, get_entities function:

    def get_entities(self):
        """Get all entities"""

        class ListWithTotalCount(list):
            """A list with the additional property total_count"""

            def __init__(self, total_count):
                super(ListWithTotalCount, self).__init__()
                self._total_count = total_count

            @property
            def total_count(self):
                """Count of all entities"""
                if self._total_count is None:
                    raise PyODataException('The collection does not include Total Count '
                                           'of items because the request was made without '
                                           'specifying "count(inline=True)".')

                return self._total_count

        def get_entities_handler(response):
            """Gets entity set from HTTP Response"""
            [...]
            if isinstance(entities, dict):
                total_count = int(entities['__count']) if '__count' in entities else None
                entities = entities['results'] # pre-existing line

            self._logger.info('Fetched %d entities', len(entities)) # pre-existing line

            result = ListWithTotalCount(total_count)
            [...]

Here's an example and the output from a sample service with the entity 'Landmark':

get_request = client.entity_sets.Landmark.get_entities().count(inline=True)
result = get_request.execute()
print(result)
print(result.total_count)

get_request2 = client.entity_sets.Landmark.get_entities()
result2 = get_request2.execute()
try:
    print(result2.total_count)
except pyodata.exceptions.PyODataException as ex:
    print(str(ex))

get_request3 = client.entity_sets.Landmark.get_entities().count()
result3 = get_request3.execute()

print(result3)
[(Name='Alexander Nevsky Cathedral',City='Sofia'), (Name='Etar Ethnographic Complex',City='Gabrovo'), (Name='National Palace
 of Culture',City='Sofia'), (Name='National Theatre',City='Sofia'), (Name='Shipka Monastery',City='Stara Zagora'), (Name='Sh
ipka Monument',City='Stara Zagora'), (Name='Sveta Nedelya Church',City='Sofia'), (Name='The Eyes of God Cave',City='Karlukov
o')]
8

The collection does not include Total Count of items because the request was made without specifying "count(inline=True)".
8

At this point, I think after writing the test, the inlinecount functionality will be pretty much finished. Just a few questions/notes:

  1. You mentioned to implement the List interface. Python doesn't really have the interface as understood in e.g. Java, so I've inherited from List instead. Did you mean something else?
  2. Currently, ListWithTotalCount is defined inside the get_entities() function. Is there a better way to position it?
  3. I couldn't find an exception called "ProgramError", so I currently have the top-level PyODataException.
filak-sap commented 4 years ago

Your code looks good to me.

1.

We don't need writable list and thus I didn't think we need to inherit the standard list. Python does not have formal interfaces but interfaces are there - just add the same methods as the standard list have and leave out those we don't need/want.

However, your way is OK because pyodata already returns list and replacing it with something else would break API promises.

2.

I would just create it as a solo class for easier re-use and testing.

3.

Correct. It does not exist yet. You need to add the exception class.


Fell free to open a preview Pull Request or share commits in your forked repo. You don't need to manually copy'n'paste the changes.

s7oev commented 4 years ago

I've added the ListWithTotalCount as a separate class, and created the ProgramError exception class. Also created tests for the following cases:

Here's a compare between my forked repo and this repo.

I haven't added as much tests as there are for $count. The ones I've not added are about different operators and combinations of $filter. But as $inlinecount is simply a query parameter (as opposed to $count which is a resource path in itself), I feel these are unnecessary.

filak-sap commented 4 years ago

Well done! I think 2 commits would be enough:

s7oev commented 4 years ago

Sounds great! Just made a pull request with the named 2 commits

filak-sap commented 4 years ago

Fixed by #122

s7oev commented 4 years ago

@filak-sap perfect! Thanks for the support and ideas on realizing this.