[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: |
|
||||||||||||||||
| 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
|
| 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 From Adam Dickmeiss From Marc Johnson |
| 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:
(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. |
| 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
|
| 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 ] |
| 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: 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
|