Add ability to search library by old 5 digit library number

[seankwilliams]

Change the library search so, in addition to the current fields it looks at, it will also consider the value at custom.library_number

[kmid5280]

@seankwilliams I'm trying to duplicate this, and it seems to work okay in the dev environment. If I create a new album with a value for the Library Number field and then search that number, it will come up in the search results. I also tried entering a 5 digit number at random in the search bar and if there is an album with a corresponding number in the Library Number field, it will show. For instance I entered the number 53028 and it returned an album with that library number. Do you suppose this might only be happening on the live page? Let me know if I'm missing anything.

[seankwilliams]

@kmid5280 I just dug deeper on this and the problem seems specific to the search on Show Builder. The search on the music library is properly returning the album based on library number search. Can you compare what's different between those two and see if the Show Builder search can search by library number as well?

[kmid5280]

@seankwilliams Do you suppose this might be happening because the Library Search Page is searching through all available information, whereas the Show Builder search is only searching through the track information (which would cause it to skip over the album information?)

[seankwilliams]

@kmid5280 yes, that's a great thought and that very well could be what's happening here. If that is the case, you'd have to modify the Mongo query on the back-end so it also considers searching the track's related album's library number field -- which is easy to say, but kind of a challenging query to write with how Mongo makes you query related entities.

[kmid5280]

@seankwilliams What about if we had the search function the same way as the one on the Library Search page, but have it filter the tracks only?

[seankwilliams]

@kmid5280 That could work. I believe the Show Builder search has some logic that combines the search results from the library with search results from the iTunes API, which includes some tracks that are not entered into Comrad. If I recall correctly, combining the iTunes API search logic with the same way the library search page searches is probably an easier route than querying library number with the existing query.

[seankwilliams]

@kmid5280 just to help on some of the logic, the show builder is calling the search API at:

client\src\pages\ShowBuilderPage\ShowBuilderPage.js

line 490: libraryActions.search('track', form.q, null, null, 30, null, null, true);

Which in turn calls the search in the file at client\src\redux\library\actions\search.js

Which in turn calls the API on the back-end at server\v1\controllers\library\search.js

The challenge with searching everything and filtering down to tracks is that the search results will have to respect the limit passed into the search API, which I believe is 30 results. If I recall correctly, the API limits the responses to 30 results but fill sin the remainder with additional results from the iTunes API if fewer than 30 results are in the Comrad database.

[kmid5280]

@seankwilliams If I add the following code to server\v1\controllers\library\search.js in the "track" switch case (around line 233 on my end), it will trigger correctly and log the old library number in the console if present:

console.log(result.album.custom.library_number)

I'm wondering if there's a place in here we can put a conditional that will include that track in the results if the track happens to contain one of those numbers? Or is this the right file to be attempting that in?

[seankwilliams]

@kmid5280 By that point in the code, the results have already been returned from Mongo.

This is the code that does the library search:

const libraryResults = await db.Library.find(
    filterObj,
    {
      name: 1,
      artists: 1,
      artist: 1,
      album: 1,
      popularity: 1,
      type: 1,
      updated_at: 1,
      search_index: 1,
      score: { $meta: 'textScore' },
    },
    {
      sort: { score: { $meta: 'textScore' } },
      limit: limit, //don't limit these since this query does not score based on a combo of track + artist + album name
    },
  ).populate(['artist', 'artists', 'album']);

I believe that code is doing a search on a search_index field, but my Mongo is rusty so I don't know for sure. You could look at the Mongo documentation to understand how this code runs:

let filterObj = { $text: { $search: searchString } };

Then, see if you can find how Comrad updates the search_index field and add the library number to that field for tracks.

Here's the note on that field from the library model. Seems like a good place to add the custom library number for a track:

search_index: {
      //will include all fields that should be searched. this will bring related entities onto this field, like artist/album names
      type: String,
    },

[kmid5280]

@seankwilliams I'm not finding anywhere in the code that governs which fields are searched depending on the type that is being searched, i.e. whether you are searching for an album, an artist, or a track. If I search for a library number that I know exists, it will return the correct results on the Library Search Page if I set the filter to Albums, but not if I set the filter to Artists or Tracks. At the same time, there doesn't seem to be any conditional anywhere that specifically instructs the find method to search for the library_number field, in any case.

I do see there is a generateSearchIndexForLibrary and an updateSearchIndex page, but I'm not really sure where these run or what triggers them. They won't generate console logs when I start the app or run searches. They also do not reference specific searchable fields.

Do you suppose the issue might be that the library number is not included as as a property in the Schema for tracks? I do see that in the Library Schema, there is a custom: Schema.Types.Mixed which I presume may include the library_number property, though it looks like this is included under artists, albums, and tracks.

[seankwilliams]

@kmid5280 I'll write up some details on how the search index field works when I have a few minutes. Might take a few because of the holidays here, but I'll put some info together that I think will help with this.

[seankwilliams]

@kmid5280 At long last, here's some information on how the search index works:

First, you'll see in server\v1\controllers\library\search.js that we are using a MongoDB full text search:

If you check https://www.mongodb.com/docs/manual/reference/operator/query/text/, you'll see that the $text field does a search on the fields that are in a text index on the model.

So, the next step is to check the model and see what fields are in the text index. Looking at server\v1\models\library.js, you'll see:

That is putting the following fields in the text index:

• name
• search_index
• Any custom fields that have been designated as being included in the text index - this is anything in keys.modelCustomFields['album'] that has includeInTextIndex set to true

If you look into those custom fields (server\v1\config\base.js), you'll see that library_number is flagged to be included in the search index. However, the library number would only appear on albums, and we need to search by it when we are searching tracks. One possibility is to add the library number to a track, but it's really a data point for albums. If we put the same data on tracks, then it would have to get updated every time the corresponding album is updated.

Instead, let's look at the search_index field. If you check the comment associated with this field in the model (server\v1\models\library.js), you'll see it says:

//will include all fields that should be searched. this will bring related entities onto this field, like artist/album names

This is exactly what we want to do, bring the related album's library number onto this field for a full text search. Next up, we'll need to see where search_index gets set. For things like that, it's helpful to use your IDE to search all files. Searching for "search_index", you can see two files that set this field:

• server\v1\controllers\library\utils\updateSearchIndex.js
• server\v1\seedDB\index.js

That first file is the key one we need to update. For tracks, we'll want to add the album's library number into the search_index field. The updateSearchIndex function is triggered when a library object gets updated, so once you make the change, to test it, you'll either have to update a single record and test searching for that record, or we can bulk update every library object in the database. To do that, you'll execute the file at server\v1\dataprocessing\generateSearchIndexForLibrary.js, which can be done using npm run generate-search-index (package.json contains the insructions that map npm run generate-search-index to that file)

That second file is only used for initial seeding of the database. It's still good to update that, just so the search index will be correctly generated when devs seed a local database.

[kmid5280]

@seankwilliams Thanks for this explainer. In server\v1\controllers\library\utils\updateSearchIndex.js, do you suppose we will need to add a new case for the tracks, and set it to look in the album field for the album number? I see currently there is only a case for the artist and for the album.

[seankwilliams]

@kmid5280 Yes, let's add a new use case for tracks and set it up as you describe!

[kmid5280]

@seankwilliams I'm currently trying to understand how this works. Do you suppose if we write the first line of the 'track' case as the following, that it will fit the album library number into the search? It seems like, if tracks are being searched, we want it to search for albums and specify the custom library number. I'm having some trouble using console.log here to see if I'm identifying the library number correctly.

return db.Library.find({ type: 'album', album: libraryDoc.custom.library_number })

[seankwilliams]

@kmid5280 Can you send the steps you are using to try to test the function? That might help with identifying why you aren't seeing results with console.log.

As for this code:

return db.Library.find({ type: 'album', album: libraryDoc.custom.library_number })

What line are you thinking of adding it? This code would search the Library model for albums that match the library number you are specifying.

[kmid5280]

@seankwilliams I was thinking of adding that to line 64, just creating a new case for tracks. I assume that after I add this new line, I'll have to test it by updating one of the library objects? So after I add the new case, can I test it by creating a single new track and then searching for it via its associated album number?

[seankwilliams]

@kmid5280 If you're referring to adding that to line 64 of code in server\v1\controllers\library\utils\updateSearchIndex.js, then I'd look a little closer at this file to see what it's doing. That section of the code is not the section responsible for updating the search index. Instead, it is relating other entities that should have the search index updated because they reference data points from the other entity. (Like, if an artist name updates, the album and tracks also need to update, because they use the artist name in the search index.)

[kmid5280]

@seankwilliams Thanks. Do you suppose instead we need to change the parameters on line 35, to include the album library number there?

[seankwilliams]

@kmid5280 That's still part of the code that finds related entities to update:

These are the three lines that save the updated search index:

I'd go through that file line by line and see if you can learn what each line is doing. You'll want to understand mongoose and JavaScript promises to see what it's doing.

[kmid5280]

@seankwilliams Do you suppose we'll have to make any changes to the getSearchIndexForLibrary method at server\v1\models\library.js? As in, add a conditional to include the album library number there if we are searching via tracks. Then we would have to update the library objects via the npm command to test it. I'm wondering if we actually need to change anything in server\v1\controllers\library\utils\updateSearchIndex.

[seankwilliams]

@kmid5280 , yep, that is correct! The getSearchIndexForLibrary method is the core function that has to change.

[kmid5280]

@seankwilliams So what about a conditional like the following, starting at line 219:

if (libraryDoc.album != null && libraryDoc.custom.library_number != null) { libraryLookups.push( Library.findById(libraryDoc.album).then(a => { return a != null ? a.name : ''; }) ) }

I see that the album property is used by tracks, so I'm telling it to check if there's an album, as well as a custom library number. Since we want to add the album to the list, that's why we would be adding the album itself via findById. Does it sound like this is on the right track to you?

[seankwilliams]

@kmid5280 That's getting close. There's two things you want to look at though:

• You are checking to see if libraryDoc has both an album and a custom.library_number value. I'd check a database record and verify these would both of these exist for the entities you want to update.
• This would add the album name to the search index rather than the custom library number.

[kmid5280]

@seankwilliams If I go to the Show Builder page, search for a track, console.log the result, and look at the object with the track data, I see that it has a type: 'track' property, an album property, and possibly an album.custom.library_number, all of which would seem relevant.

So what if we update the search parameters to the following? If we are searching by tracks, and the track will show up if it has an associated album and album.custom.library_number value, is there any reason not to include those? Would you suggest searching for all three properties?

if (libraryDoc.type == 'track' && libraryDoc.album != null && libraryDoc.album.custom.library_number != null) { libraryLookups.push( Library.findById(libraryDoc.track).then(a => { return a != null ? a.name : ''; }) ) }

[seankwilliams]

@kmid5280 I think you're missing a few pieces on how the search is working as a whole and the purpose of librarySchema.methods.getSearchIndexForLibrary. I'd review the comment I have above (https://github.com/codefordenver/Comrad/issues/875#issuecomment-1910969585) outlining how the search works. The code that you're suggesting changing is updating the search index, and in this case, you would be adding the album name to the search index, and it's already in the search index:

then(a => { return a != null ? a.name : '';

[kmid5280]

@seankwilliams Thanks for the comment. I'll go back and review some of this material then.

Couple questions: in server\v1\models\library.js, what do libraryDoc and libraryLookups on lines 207 and 208 respectively refer to? Is libraryDoc referring to a specific item in the collection being referenced, i.e. a track, album, etc.? It looks to me like the conditionals under it are specifying what to do if the particular item is an album, artist, and so on. And is libraryLookups basically the entirety of all of the search results?

[seankwilliams]

@kmid5280 Good questions! Here's the declarations of each of those:

Since libraryDoc is this, it means it's whatever object the method is being called on. So, we'll go see where getSearchIndexForLibrary is being called in server\v1\controllers\library\utils\updateSearchIndex.js:

That doesn't help still, because libraryDoc is being passed as a parameter. So, you'll have to see where updateSearchIndex is being called. This is used often in the project, so let's look at this part from the API request to update a library item in server\v1\controllers\library\update.js :

This does the following:

• Find one item from the library model that matches the id value in the API request (db.Library.findOne({ _id: req.params.id }))
• That item becomes libraryData once the promise returns (.then)
• Get libraryData.artist and libraryData.name and put them in variables
• If libraryData.type is album:
  • First, validate the album data. This is a separate asynchronous function, so once it runs, its result will be returned to albumData in .then(albumData => (in this case, looking at the code, it is not clear what exactly albumData would contain. You'd have to go into the validateAlbumData to see what it returns. It's a moot point though since that variable is not used
  • Then, we run db.Library.findOneAndUpdate. This is where we actually update the database. Note that this has a parameter of new: true in it, which means, when this promise resolves, the new library item is going to be returned.
  • The next relevant line here is .then(dbAlbum => updateSearchIndex(dbAlbum)) dbAlbum is the return value from the previous promise, which is the newly updated database record for the album.

libraryLookups

To understand this one, you've got to grasp asynchronous programming with JavaScript. I'm frankly weak at that - I find it hard to read and don't entirely understand the reasoning behind why Node.JS relies so heavily on asynchronous JavaScript. But to write Node you've got to have a handle on asynchronous JS otherwise it's way too easy to get lost. So, if you aren't up to speed with promises and async functions, I'd look up some resources on that and read up on it.

libraryLookups is an array of promises that have to resolve. In this case, we are doing up to three database lookups (each time libraryLookups.push happens, the parameter for push is a promise that is a Mongo database lookup). Mongoose is asynchronous, so we can't just do, for example, let albumName = Library.findById(libraryDoc.album).then(a => a.name). If that was synchronous, on the line immediately following, albumName would equal the actual album name. But it's asynchronous, so `albumName is a promise that probably is not yet resolved.

Finally, we get to this part of the code:

return Promise.all(libraryLookups)
    .then(values => {

That says "wait until all of the promises we started are done". Then, the results of each promise is returned into an array of values.

[kmid5280]

@seankwilliams I'm going back over some of the previous comments. Am I correct that this is structured so that when you update an item in the library (say an album, track, etc.) by editing it, this also triggers an update to the search index afterwards via a promise so that it will show up in a search? I want to conceptually understand where we are updating the item's content versus updating the search index.

[seankwilliams]

@kmid5280 yep, that is absolutely correct!

[kmid5280]

@seankwilliams Tell me if this sounds right - ideally, if we update an album, and the album contains a library number, this should trigger the search index update so that that library number is then included in the index moving forward. Then, it will show up whenever we search for tracks too. Therefore we don't have to include any specific logic for tracks in getSearchIndexForLibrary, we simply need to add a conditional that will include the library number in the index. Does that sound like it's on the right track?

[seankwilliams]

@kmid5280 yes, that sounds like that's on the right track! Without digging deeper I am not sure if you'd need to add the library number to both the search index for tracks and the album, or just for one or the other. The track might need the change - it depends on which library type the search is looking at.

[kmid5280]

@seankwilliams Going back to your earlier comment, how do you test a database record to see if it contains the fields I want the index to update? Can I just console.log a search result and verify that the fields are in there that way?

[seankwilliams]

@kmid5280 Yep, that's one way to do it. The other ways are to (1) download and use MongoDB Compass, which is a UX around Mongo DB, or (2) connect to Mongo via command line and query for the record you want to check out.

[kmid5280]

@seankwilliams Below is a console.log of a track result from Show Builder that shows an 'album' type and a custom attribute:

So what if we make the conditional in getSearchIndexForLibrary the following? I'm thinking this would check for an album type, the presence of a library number, and then push the library number to the search index. If I'm not mistaken we would then have to update the library objects in order to include the library number in the index. How does that sound?

if (libraryDoc.album != null && libraryDoc.album.custom.library_number != null) { libraryLookups.push( Library.findById(libraryDoc.album).then(a => { return a != null ? a.custom.library_number : ''; }) ) }

[seankwilliams]

@kmid5280 This is on the right track. I would check to be sure that libraryDoc.album is populated or not. In the database, it's just the object ID of the album the record links to. Some queries on the back-end populate related records and others do not.

If libraryDoc.album is just an ID, then libraryDoc.album.custom.library_number will never be set, even if the album has a custom.library_number attribute.

In the code you've got, this part of the code is supposed to look up the full album object based on libraryDoc.album being an ID: Library.findById(libraryDoc.album)

[kmid5280]

@seankwilliams Are you saying that the conditional libraryDoc.album.custom.library_number != null isn't necessarily going to check if the custom.library_number attribute is populated with a value, and could return true if it sees that the attribute simply exists? Do we need a different conditional here to verify that there actually is a value?

[seankwilliams]

@kmid5280 It depends on the database query used to retrieve libraryDoc.

Here's a query out of MongoDB Compass for library records that have an album property:

As you can see, album is just an ObjectId. That is the object id of a different library record for the album.

So, on the Node.js side, libraryDoc.album could be an object id, or it could be the actual album. It depends on whether the Mongoose queries have already populated the related entity.

The code in getSearchIndexForLibrary in the models/library.js file is assuming that the related entities like libraryDoc.album and libraryDoc.artist are only artist IDs, so it's using Library.findById promises to find the related entity. If you want to get the custom.library_number associated with libraryDoc.album, you'll have to look in the response of a Library.findById promise that retrieves the library record associated with libraryDoc.album.

[kmid5280]

@seankwilliams When you say to look in the response of a Library.findById promise, do you mean for instance adding a conditional within the Library.findById(libraryDoc.album).then(a => { block that would check for it? For instance, something like:

if (libraryDoc.album != null && libraryDoc.album.custom.library_number != null) { libraryLookups.push( Library.findById(libraryDoc.album).then(a => { if (a != null) { if (a.custom.library_number != null) { return a.custom.library_number; } } else { return ''; } }) ) }

[seankwilliams]

@kmid5280 Let me ask you this: can you explain why libraryLookups is necessary here? It is necessary, but understanding why is key to this part of the code.

[kmid5280]

@seankwilliams If I understand right, libraryLookups is an array of promises that get collected, and then run together in the return Promise.all statement. I think we are doing it this way to centralize the updated values (which rely on information returned from promises), because these library objects don't exist in a vacuum. Updating information in an album could also affect its related artists and tracks, so we have to be able to make those updates as well. Is that along the lines of what you're looking for?

[seankwilliams]

@kmid5280 Your explanation actually relates to the code in server\v1\controllers\library\utils\updateSearchIndex.js, which updates the search indexes for all information related to the library entities (updating an artist would update its albums and tracks, etc.).

As far as the library lookups code in server\v1\models\library.js, the code looks like this:

  let libraryLookups = [];

  if (libraryDoc.album != null) {
    libraryLookups.push(
      Library.findById(libraryDoc.album).then(a => {
        return a != null ? a.name : '';
      }),
    );
  }

  if (libraryDoc.artist != null) {
    libraryLookups.push(
      Library.findById(libraryDoc.artist).then(a => {
        return a != null ? a.name : '';
      }),
    );
  }

  if (libraryDoc.artists != null && libraryDoc.artists.length > 0) {
    for (var i = 0; i < libraryDoc.artists.length; i++) {
      libraryLookups.push(
        Library.findById(libraryDoc.artists[i]).then(a => {
          return a != null ? a.name : '';
        }),
      );
    }
  }

  return Promise.all(libraryLookups)
    .then(values => {
      var searchIndex = libraryDoc.name + ' ' + values.join(' ');
      return searchIndex;
    })

But why couldn't we just do this, which would be much simpler code-wise?

  let searchIndexValues = [];

  if (libraryDoc.album != null) {
    searchIndexValues.push(libraryDoc.album.name != null ? libraryDoc.album.name : '');
  }

  if (libraryDoc.artist != null) {
    searchIndexValues.push(libraryDoc.artist.name != null ? libraryDoc.artist.name : ' ');
  }

  if (libraryDoc.artists != null && libraryDoc.artists.length > 0) {
    for (var i = 0; i < libraryDoc.artists.length; i++) {
      searchIndexValues.push(libraryDoc.artists[0].name != null ? libraryDoc.artists[0].name : ' ');
    }
  }

  return libraryDoc.name + ' ' + searchIndexValues.join(' ');

[kmid5280]

@seankwilliams I think I'm making some progress on this. First to answer your question, we need to have libraryLookups written this way because we are using findById to get the album data, which requires the use of a promise. The code is checking the track to see if there is an associated album ID, and if so, using findById to retrieve the data from that album.

(This confused me at first because I thought if (libraryDoc.album != null) is a conditional that runs if the album is being updated, and only applied to albums. But it looks like this is actually checking to see if the track whose search index needs updating has an associated album ID, so that the album name can be added to the track's search index.)

I was able to get the library number added to the track's search index by rewriting the following function. There are two things I'm running into with this. One, this could probably be consolidated without having to run findById twice, though I'm not sure how to return two separate values with one return statement. Two, in Show Builder, I'm still not able to search for the track via the library album number even though the number is in the track's search index.

if (libraryDoc.album != null) {
    libraryLookups.push(
      Library.findById(libraryDoc.album).then(a => {
        return a.custom.library_number != null ? a.custom.library_number : '';
      })
    )

    libraryLookups.push(
      Library.findById(libraryDoc.album).then(a => {
        return a != null ? a.name : '';
      }),
    );
  }

[seankwilliams]

@kmid5280 Very good work, this is spot on!

Regarding this one:

One, this could probably be consolidated without having to run findById twice, though I'm not sure how to return two separate values with one return statement.

Yep, you're correct. The search index is saved to the database as a list of words separated by spaces, so you can do something like this:

Library.findById(libraryDoc.album).then(a => {
        let returnValue = '';
        returnValue += a.custom.library_number != null ? a.custom.library_number : ''
        if (a != null) {
          if (returnValue.length > 0) {
            returnValue += ' ';
          }
          returnValue += a.name;
        }
        return returnValue;
})

As for the Show Builder search, I'm not sure what's going on there. Can you push your branch and I'll test it locally on my end to see?

[kmid5280]

@seankwilliams I verified that your version of the code is returning the correct values and adding them to the search index, so I went ahead and went with that. Pushed it and made a PR. I'm testing it by going to the Show Builder and searching for a track via its album library number. Currently it's not showing up so wondering if there's another step in this?

[kmid5280]

@seankwilliams Sorry, not sure if the PR was necessary at this stage, I can close it if not.

[seankwilliams]

Thanks @kmid5280 ! Making a PR is fine, I'll wait to merge it until we're sure it's working. I'll test that out! Hopefully soon, but it may take me a few days.