[FOLIO-996] Codex: Use totalRecords from resultInfo Created: 21/Dec/17  Updated: 12/Nov/18  Resolved: 05/Jan/18

Status: Closed
Project: FOLIO
Components: None
Affects versions: None
Fix versions: None

Type: Task Priority: P3
Reporter: Adam Dickmeiss Assignee: Hongwei Ji
Resolution: Done Votes: 0
Labels: alpha, core, sprint29
Remaining Estimate: 2 hours
Time Spent: 3 hours, 45 minutes
Original estimate: Not Specified

Issue links:
Blocks
blocks STCON-55 Enable RESTResource to get totalRecor... Closed
blocks MODCXEKB-54 Change totalRecords to resultInfo.tot... Closed
blocks MODCXMOCK-14 implement resultInfo Closed
Sprint:

 Description   

At the same time remove totalRecords from instanceCollection.json

This is a breaking change to the shared raml



 Comments   
Comment by Adam Dickmeiss [ 21/Dec/17 ]

This is depending on MODCXMUX-7 Closed which as makes codex includes resultInfo .

Comment by Adam Dickmeiss [ 21/Dec/17 ]

Mike Taylor .. would it be a big deal to change ui-search to use totalRecords from resultInfo/totalRecords rather than totalRecords (top-level). The latter which would be removed by this change.

Comment by Mike Taylor [ 21/Dec/17 ]

We can do it; but do we want to? There is a standard that all our searchable backend modules adhere to. Making the mod-codex-mux diverge from this standard seems like something we should do only if absolutely necessary.

Comment by Hongwei Ji [ 21/Dec/17 ]

Mike Taylor and Jakub Skoczen Below messages are from Codex slack discussion. Can we confirm which one (totalRecords or resultInfo.totalRecords) is the long term goal?

From Marc Johnson
@dickmeiss I believe the intention was that the `totalRecords` properties in each collection schema would eventually be replaced by an object property that used resultInfo
Posted in #codexToday at 9:22 AM

From Adam Dickmeiss
ok.. If that is the intention the "move" is a breaking change (ie remove a required totalRecords field from top-level).
Posted in #codexToday at 9:25 AM

From Marc Johnson
I believe so, it might be worth checking with @jakub that is still the plan for interfaces going forward
Posted in #codexToday at 9:26 AM

Comment by Adam Dickmeiss [ 21/Dec/17 ]

Can we at least agree that having two places for totalRecords is misleading?

I don't care where we put it.

Comment by shale99 [ 21/Dec/17 ]

what marc stated is correct, the only reason the original totalRecords has remained is because module developers were given time to introduce this breaking change at an appropriate time - however, it seems like no one has made this change - the intention is to remove this, and this code will be depreciated in rmb at one point at right now there is support for both and this will not be maintained forever

Comment by Mike Taylor [ 22/Dec/17 ]

OK, I misunderstood what's going on here. You're not wanting to change mod-codex-mux to behave different from other RMB modules; you're using the RMB support to do the standard thing, which at present is to put the count in both places. Somehow I missed this change when it was made.

I guess we should file issues to update ui-inventory, ui-items, ui-requests and <SearchAndSort> to get the count from the new location; then, after they're fixed, we can go ahead and remove it from the old location.

This feels like an annoying waste of time, especially as the name of the field has already changed once (from total_records), with attendant dancing around keeping the UI modules compatible.

Comment by Hongwei Ji [ 03/Jan/18 ]

Adam Dickmeiss I updated your branch https://github.com/folio-org/raml/tree/modcxmux-7 to remove totalRecords and only use resultInfo.totalRecords. I also have codex-ekb ready locally for this breaking change. Let me know when do you want to merge to RAML master.

Comment by Heikki Levanto [ 05/Jan/18 ]

I think removing totalRecords at this point is risky, it is a breaking change that affects many modules. I think we should just deprecate it for a few versions, and remove it completely when alpha is out.

Comment by Mike Taylor [ 05/Jan/18 ]

And predictably enough, it has broken the UI module. sigh

Not to worry. I'll fix the UI Codex Search module to work around the breakage.

Comment by Adam Dickmeiss [ 05/Jan/18 ]

Mike Taylor ?? How is that possible? Neither mod-codex-mux nor mod-codex-inventory is using that new raml-utils .

Comment by Mike Taylor [ 05/Jan/18 ]

I don't know – Just know that ui-search is failing with "Cannot read property 'totalRecords' of undefined"

Will investigate more after lunch.

Comment by Hongwei Ji [ 05/Jan/18 ]

Heikki Levanto I believe we only focus the change within Codex modules (codex-mux, codex-mock, codex-ekb, codex-inventory, and of course ui-search), so the risk is relatively low and contained.

Comment by Mike Taylor [ 05/Jan/18 ]

OK, my problem seems to be caused by something even weirder:

GET http://localhost:9130/codex-instances?limit=30&query=cql.allRecords%3D1%20sortby%20title

500 Internal Server Error
Module mod-codex-inventory-0.0.4-SNAPSHOT.17 500
Unknown index: cql.allRecords
Module mod-codex-mock-1.0.4-SNAPSHOT.24 200
{
  "instances" : [ {
    "id" : "77777777-7777-7777-7777-000000002016",
    "title" : "1, 2, 3",
    "contributor" : [ ],
    "publisher" : "Dorling Kindersley, 2000",
    "type" : "books",
    "identifier" : [ {
      "value" : "0751362484",
      "type" : "isbn"
    } ],
    "source" : "kb",
    "language" : [ ]
  }, 
  ...
  ],
  "totalRecords" : 204
}
Module mod-codex-ekb-0.0.2-SNAPSHOT.52 500
Configuration data is invalid

So it seems that a bunch of instances are being correctly returned, but I am also getting a bunch of error messages preceding and following the data. Surely that can't be right.

Also: the specific error reported here ("Unknown index: cql.allRecords") makes no sense, as the query cql.allRecords=1 sortby title does not use that index.

Please advise.

Comment by Mike Taylor [ 05/Jan/18 ]

When I change the parameters to do a title search (title="a*" sortby title), I get something similar:

GET /codex-instances?limit=30&query=title%3D%22a%2A%22%20sortby%20title

500 Internal Server Error
Module mod-codex-mock-1.0.4-SNAPSHOT.24 200
{ /* list of records from mock */ }
Module mod-codex-ekb-0.0.2-SNAPSHOT.52 500
Configuration data is invalid
Module mod-codex-inventory-0.0.4-SNAPSHOT.17 200
{ /* list of records from inventory */ }

My interpretation: when an error occurs in any back-end, mux generates a response that includes, for each provider:

  • the name of the provider module
  • either the data it returned or the errors it reported

(And if any of the back-end modules yielded an error, it yields an error status: not sure if it's always 500, or if it uses the error status from the back-end module.)

If I am right, then the good news is that we are successfully multiplexing the mock and inventory codex modules; the bad news is that an error in mod-codex-ekb is making it impossible to get a good response.

Are there any queries that we know do work in mod-codex-ekb?

Comment by Hongwei Ji [ 05/Jan/18 ]

Mike Taylor the ekb error says "Configuration data is invalid", so probably not a query issue.

Comment by Mike Taylor [ 05/Jan/18 ]

Thanks, Hongwei Ji. I'll meet you on Slack to discuss.

Comment by Mike Taylor [ 05/Jan/18 ]

Clarification on what's going on, then. Two separate back-end probems.
1. "Configuration data is invalid" in mod-codex-ekb, which I am discussing on Slack with Hongwei Ji
2. cql.allRecords=1 not supported in mod-codex-inventory. I added it to the Codex CQL schema in https://github.com/folio-org/raml/pull/61 and Adam is planning to implement it.

Comment by Mike Taylor [ 05/Jan/18 ]

Based on discussion on the FOLIO Slack's #codex channel, we are not making the change in #2 after all – it would be difficult for the EBSCO KB. Instead, I will file a new issue on myself to have the Codex not include a query at all, initially.

Comment by Mike Taylor [ 05/Jan/18 ]

And #1 is probably due to the module not having been given credentials for accessing the EBSCO KB. So I will file a new issue to get that working, and I think that means all the problems I reported have "resolutions" now ... by which I mean I will fix them in the two new issues

BTW., none of this was really much to do with the ostensible purpose of FOLIO-996 Closed , so sorry for hijacking the thread. I'll leave now

Comment by Hongwei Ji [ 05/Jan/18 ]

That's great. Mike Taylor I will mark this ticket as closed now.

Comment by Hongwei Ji [ 05/Jan/18 ]

https://github.com/folio-org/raml/pull/60

Comment by Mike Taylor [ 08/Jan/18 ]

So aside from my digression (for which, my apologies) was the outcome of this issue that we now have two redundant copies of the totalRecords count in search responses?

Comment by Hongwei Ji [ 08/Jan/18 ]

Mike Taylor Just one and it is resultInfo.totalRecords.

Comment by Mike Taylor [ 08/Jan/18 ]

Hmm. Then I am surprised my UI code is still working. Maybe someone fixed stripes-connect to deal with this while I wasn't looking.

Comment by Mike Taylor [ 08/Jan/18 ]

Nope ... the code is the same as it always was:
https://github.com/folio-org/stripes-connect/blob/master/RESTResource/RESTResource.js#L499-L500

Oh, and I see now that my UI is saying "NaN Records found"

Comment by Mike Taylor [ 08/Jan/18 ]

I'll fix this in STCON-55 Closed .

Generated at Thu Feb 08 23:10:03 UTC 2024 using Jira 1001.0.0-SNAPSHOT#100246-sha1:7a5c50119eb0633d306e14180817ddef5e80c75d.