-
Notifications
You must be signed in to change notification settings - Fork 0
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
Package updates, use RCI source-mapper #415
Conversation
Update vulnerable packages. Also updates where we pull nypl-source-mapper util from - previously discovery-store-models (deprecated), now RCI. The implementation in RCI is more secure and has a major API change in that it works async, so the integration here had to change in several places (sync functions become async, and related fallout). https://newyorkpubliclibrary.atlassian.net/browse/SCC-4328
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good! Just some clarifying questions. I'm wondering if we want to make the source mapper its own npm module? Seems a bit overkill to require the entire RCI for a single file with no dependencies besides axios.
statements () { | ||
const stmts = JsonLdItemSerializer.prototype.statements.call(this) | ||
async statements () { | ||
const stmts = await JsonLdItemSerializer.prototype.statements.call(this) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this line become this.statements() instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think one could use super.statements()
at this point, but really I think this whole file needs to be rethought with no/flatter inheritance. That's a bigger effort beyond the scope of this PR, which is mainly about resolving vulnerabilities, so I'd like to punt on that kind of work if that sounds alright.
* Search Results: Agents | ||
*/ | ||
|
||
class AgentResultsSerializer extends SearchResultsSerializer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this no longer used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It hasn't been used for about 5 years and will likely take another form entirely with Contributor Explorer, so taking the opportunity to remove it.
lib/ownership_determination.js
Outdated
@@ -1,10 +1,8 @@ | |||
const NyplSourceMapper = require('discovery-store-models/lib/nypl-source-mapper') | |||
// NYPL item ids start with an 'i' | |||
const NYPL_ITEM_ID_PATTERN = /^i\d+/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this hardcoded change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a pragmatic choice. The process of deriving owner from an identifier has been made async. It was previously sync because it was using a request-sync library, which was always dangerous. Now that it's async, anything that needs to derive owner from an identifier using the official method is necessarily async. This specific use of the mapper util (isItemNyplOwned
) is used throughout the codebase in places that would be awkward to change to async. I think this specific use of the nypl-source-mapper util is unnecessary because the nypl-source-mapper util is primarily meant to future proof as we add/remove partner institutions; A method like isItemNyplOwned
depends on the identifier convention for NYPL, which is considerably more stable. Were we to change the identifier convention of NYPL, this would be one of several places we'd need to update, but I think 1) that's unlikely to happen in any known time and 2) implementing it as a sync regex check allows us to change far less in this PR, which is mainly about dependency updates.
@@ -259,8 +259,6 @@ module.exports = function (app, _private = null) { | |||
app.logger.debug('Resources#itemsByFilter', body) | |||
return app.esClient.search(body) | |||
.then((resp) => { | |||
resp = resp.body |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what made this change possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subtle API change in the move from Elasticsearch client v7 to v8
Fix small bug where items generated from holdings record checkin cards failed to be identiifed as NYPL items. This had no known impact on behavior, but betrayed the "isItemNyplOwned" function's promise
Update vulnerable packages. Also updates where we pull nypl-source-mapper util from - previously discovery-store-models (deprecated), now RCI. The implementation in RCI is more secure and has a major API change in that it works async, so the integration here had to change in several places (sync functions become async, and related fallout).
https://newyorkpubliclibrary.atlassian.net/browse/SCC-4328