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

Missing $count/inlinecount support for OData provider #287

Closed
whtan98 opened this issue Sep 20, 2019 · 16 comments
Closed

Missing $count/inlinecount support for OData provider #287

whtan98 opened this issue Sep 20, 2019 · 16 comments

Comments

@whtan98
Copy link
Contributor

whtan98 commented Sep 20, 2019

I'm submitting a Bug report

The currentOdataQueryBuilderService does not support the following option:

  1. $count (OData4)
  2. $inlinecount (OData2)

Your Environment

Software Version(s)
Angular 7/8
Angular-Slickgrid 2.10.5

Context

OdataQueryBuilderService does not support "enableCount" option

Possible Solution

Suggest to add the following to OdataQueryBuilderService .buildQuery()

 /*
    * Build the OData query string from all the options provided
    * @return string OData query
    */
  buildQuery(): string {
    if (!this._odataOptions) {
      throw new Error('Odata Service requires certain options like "top" for it to work');
    }
    this._odataOptions.filterQueue = [];
    const queryTmpArray = [];

    // When enableCount is true
    if (this._odataOptions.enableCount === true) {
      if (this._odataOptions.version === 4) {
        queryTmpArray.push(`$count=true`);
      } else if (this._odataOptions.version <= 3 ) {
        queryTmpArray.push(`$inlinecount=allpages`);
      }
    }
   //more...

Code Sample

@ghiscoding ghiscoding changed the title Missing $count/Inlinecount support for OData provider Missing $count/inlinecount support for OData provider Sep 20, 2019
@ghiscoding
Copy link
Owner

Oh I wasn't aware that OData provides total count in their query, I thought we always had to provide that ourselves in the .Net Controller. I wonder if the boolean flag is really necessary but at the same time, we could disable the pagination so I guess the flag could be useful there.

I'll take a look at that later, I thought at first this might be a breaking change but after a quick look, I guess not since user can pass whatever variable name.

Can you provide a quick sample of dataset returned after this query change? I don't have access to OData server anymore

@whtan98
Copy link
Contributor Author

whtan98 commented Sep 21, 2019

Yes. See the following V3/V4 odata services

  1. OData3
    curl 'https://services.odata.org/V3/Northwind/Northwind.svc/Customers?$top=2&$inlinecount=allpages' -H "Accept: application/json;"

  2. OData4
    curl 'https://services.odata.org/V4/Northwind/Northwind.svc/Customers?$top=2&$count=true' -H "Accept: application/json;"

@ghiscoding
Copy link
Owner

Thanks, however if I run those queries you sent, it shows the count in a strange variable. Is "@odata.count" a generic name and you can change it or is that the commonly used name and is what you get on your side as well? I need to have an answer on this since this will influence the Pagination.

{
	"@odata.context":"https://services.odata.org/V4/Northwind/Northwind.svc/$metadata#Customers",
	"@odata.count":91,
	"value":[{"CustomerID":"ALFKI","CompanyName":"Alfreds Futterkiste","ContactName":"Maria Anders","ContactTitle":"Sales Representative","Address":"Obere Str. 57","City":"Berlin","Region":null,"PostalCode":"12209","Country":"Germany","Phone":"030-0074321","Fax":"030-0076545"},{"CustomerID":"ANATR","CompanyName":"Ana Trujillo Emparedados y helados","ContactName":"Ana Trujillo","ContactTitle":"Owner","Address":"Avda. de la Constituci\u00f3n 2222","City":"M\u00e9xico D.F.","Region":null,"PostalCode":"05021","Country":"Mexico","Phone":"(5) 555-4729","Fax":"(5) 555-3745"}]
}

@whtan98
Copy link
Contributor Author

whtan98 commented Sep 21, 2019 via email

@ghiscoding
Copy link
Owner

ghiscoding commented Sep 24, 2019

@whtan98
With this change, which I'm currently working on adding, what is your response structure?

is it (I also saw that it's @odata.count in v4 but odata.count in v2)

{
   "@odata.count":91,
   "value": [{"CustomerID":"ALFKI","CompanyName":"Alfreds Futterkiste","ContactName":"Maria Anders","ContactTitle":"Sales Representative","Address":"Obere Str. 57","City":"Berlin","Region":null,"PostalCode":"12209","Country":"Germany","Phone":"030-0074321","Fax":"030-0076545"},{"CustomerID":"ANATR","CompanyName":"Ana Trujillo Emparedados y helados","ContactName":"Ana Trujillo","ContactTitle":"Owner","Address":"Avda. de la Constituci\u00f3n 2222","City":"M\u00e9xico D.F.","Region":null,"PostalCode":"05021","Country":"Mexico","Phone":"(5) 555-4729","Fax":"(5) 555-3745"}]
}

or

{
   "@odata.count":91,
   "items": [{"CustomerID":"ALFKI","CompanyName":"Alfreds Futterkiste","ContactName":"Maria Anders","ContactTitle":"Sales Representative","Address":"Obere Str. 57","City":"Berlin","Region":null,"PostalCode":"12209","Country":"Germany","Phone":"030-0074321","Fax":"030-0076545"},{"CustomerID":"ANATR","CompanyName":"Ana Trujillo Emparedados y helados","ContactName":"Ana Trujillo","ContactTitle":"Owner","Address":"Avda. de la Constituci\u00f3n 2222","City":"M\u00e9xico D.F.","Region":null,"PostalCode":"05021","Country":"Mexico","Phone":"(5) 555-4729","Fax":"(5) 555-3745"}]
}

or something different? I'd like to make sure that the Example Grid OData represent the correct behavior. I guess that items can be changed to value or anything else but I'm not sure after this code change, we used to be able to put whatever we want before this code change, would that be still true? ... if that is no longer true, then that would be a small breaking change since I have advertised items in my Wiki and in the Example.

@ghiscoding
Copy link
Owner

ghiscoding commented Sep 24, 2019

I created the PR #291, if you could review it and answer the question in previous post before I merge the code. The PR has a few files because I also updated Jest unit tests and Cypress E2E tests (the .html files are irrelevant, if you could just review the .ts files).

Note that I changed your code a bit because version is not a required property and when it's not provided it defaults to OData v2, here's the new code.

if (this._odataOptions.enableCount === true) {
      const countQuery = (this._odataOptions.version >= 4) ? '$count=true' : '$inlinecount=allpages';
      queryTmpArray.push(countQuery);
}

@whtan98
Copy link
Contributor Author

whtan98 commented Sep 25, 2019

@whtan98
With this change, which I'm currently working on adding, what is your response structure?

is it (I also saw that it's @odata.count in v4 but odata.count in v2)

I got exactly the same observations as yours.

However, looks like things may get tricky if the grid tries to consume the "count" control information, after reading the following excerpts from ODATAv4 spec https://docs.oasis-open.org/odata/odata-json-format/v4.01/csprd05/odata-json-format-v4.01-csprd05.html#_Toc14363451

See picture below:
odata

@ghiscoding
Copy link
Owner

ghiscoding commented Sep 25, 2019

From what you pointed out, that seems to mean that the result will be @count instead of @data.count because we always use a GET without the version in the header. I think that in any case, you have to deal with the total count yourself in the Component, so that would work out anyway since you'll use the name that works for you. As long as the code change for $inlinecount and $count is still what you provided then I think we are fine.

Can you validate that it works on your side. I'd like to release a new version this week. Thanks

@whtan98
Copy link
Contributor Author

whtan98 commented Sep 25, 2019

Agreed. The count control info should be handled separately.

BTW, below are my findings for the default name:

  1. V3 is "odata.count"
  2. V4 is "@odata.count"

As the odata service always specify the ODATA-Version in the RESPONSE HTTP header

See attached curl output
output.txt

Will let you know once I tested the changes.

Many thanks

@ghiscoding
Copy link
Owner

Oh I thought the text you sent for only for the Request Header but now I see it's for both Request & Response Headers. Anyhow, my lib won't explicitly change anything in the Request Header, the only thing my lib does is to build the OData query string, the user (you) do whatever you want to with the Request afterward (meaning you can choose to add the OData version in your Request if you wish but I assume most people won't because again my lib doesn't handle the http call itself).

Also, I looked back at the OData Wiki and I realized that the callback, which has to change the pagination total count property, was not shown in the OData code Wiki so I updated the Wiki, basically the following

getCustomerCallback(response) {
    // totalItems property needs to be filled for pagination to work correctly
    // however we need to force Angular to do a dirty check, doing a clone object will do just that
    this.gridOptions.pagination.totalItems = response.totalRecordCount;
    this.gridOptions = Object.assign({}, this.gridOptions);

    this.dataset = response.items as Customer[];
  }

However the OData example always had that piece of code.. and again it's up to the user (you) to handle the total count returned from the callback and update the pagination count (else the pagination will be broken).

@whtan98
Copy link
Contributor Author

whtan98 commented Sep 26, 2019

Can you validate that it works on your side. I'd like to release a new version this week. Thanks

It works correctly for me. Many thanks for the great effort

@ghiscoding
Copy link
Owner

ghiscoding commented Sep 26, 2019

@whtan98
Can you share the code of your postProcess callback? I might do improvement in the near future to auto-refresh internally (that is something I already do with the GraphQL service) but I need to make sure it's always following a certain structure.

Side note, I won't publish a new version this week, it's too late now and I found some small bugs in the Editors that I'd like to fix before releasing. So next week most probably

@whtan98
Copy link
Contributor Author

whtan98 commented Sep 27, 2019

I'm afraid I don't have any proper code for postProcess yet, as I still very much on exploring how to make use of Angular SlickGrid

ghiscoding added a commit that referenced this issue Sep 27, 2019
feat(odata): add "enableCount" flag to add to OData query, closes #287
@ghiscoding
Copy link
Owner

Guess I'll wait to do that next enhancement then, I need someone with access to OData. Thanks

@ghiscoding
Copy link
Owner

Now released under version 2.11.0

@whtan98
Copy link
Contributor Author

whtan98 commented Oct 5, 2019 via email

ghiscoding referenced this issue in jr01/slickgrid-universal Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants