apostrophecms / apostrophe

A full-featured, open-source content management framework built with Node.js that empowers organizations by combining in-context editing and headless architecture in a full-stack JS environment.
https://apostrophecms.com
Other
4.36k stars 590 forks source link

3.0: Multiple search issues #3193

Open myovchev opened 3 years ago

myovchev commented 3 years ago

Describe the bug

  1. The task node app @apostrophecms/search:index fails with TypeError: self.indexDoc is not a function
  2. Pieces types are not registered programmatically as stated in the source documentation even with explicit searchable: true settings.
  3. I'm unable to include a desired field in the search index even via explicit searchable: true field option (tested with string textarea field type).
  4. The implementation of addSearchTexts disallows any weight configuration, and there are valid cases when a specific area text field might be of type highSearchText. I haven't tested if the rich-text widgets are searchable yet, and the above is obviously not a bug.
  5. The silent option for fields is not documented anywhere and it's unclear how it would affect the search. It's definitely not a part of the search result UI, my guess is it has something to do with API result set? (again not a search related bug report)

Expected behavior

  1. It looks to be a typo. indexDoc is defined in the handlers configuration and not in methods.
  2. I'm unable to find a registered handler for the determineTypes event in the pieces source but only dedicated method self.searchDetermineTypes(types) which in turn is not invoked anywhere (but I might miss something here). The only way to enable a piece for a search is via the explicit types option of the search module.
  3. There is a trace to this feature in the schema module (indexFields method) and string field type has a dedicated index method.

Details

*Apostrophe v3.0.1*

boutell commented 3 years ago

Yes, we have some work to do here. The search mechanism is working well enough for inline search in the manage view but we need to go back and QA it from a standpoint of sitewide search. The indexDoc thing definitely sounds like it should have been a method so it could be called from this task and we just missed it.

As for incorporating your own fields directly into the mongodb index, I'm not sure if we'll be reworking that, but I take your point that it would be useful to be able to declare that a specific area's text belongs in highSearchText, especially since that is essential if you want it to be available for autocomplete purposes. This would be secondary to mopping up the clear-cut bugs you pointed out.

myovchev commented 3 years ago

Thanks Tom, it's fair enough. The most critical issues are related with enabling fields and pieces not added to the search index. The rest could come in v4 :D

abea commented 2 years ago

I've resolve those two in an upcoming PR.

@boutell Were you saying that searchable (and silent maybe as well) should not be expected to operate as the code and code comments suggest? They're deprecated A2 features? I'm finding the same thing as @myovchev. Probably the same question for addSearchTexts since it's in the code, though I haven't tested that yet.

boutell commented 2 years ago

Ah, no, searchable and silent should be expected to work, although I haven't reviewed the implementation.

On Thu, Dec 2, 2021 at 1:15 PM Alex Bea @.***> wrote:

  • The task node app @apostrophecms/search:index fails with TypeError: self.indexDoc is not a function
  • Pieces types are not registered programmatically as stated in the source documentation even with explicit searchable: true settings.

I've resolve those two in an upcoming PR.

@boutell https://github.com/boutell Were you saying that searchable (and silent maybe as well) should not be expected to operate as the code and code comments suggest? They're deprecated A2 features? I'm finding the same thing as @myovchev https://github.com/myovchev. Probably the same question for addSearchTexts since it's in the code, though I haven't tested that yet.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe/issues/3193#issuecomment-984876939, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27OKCSKN76YTI3USPE3UO6ZSNANCNFSM47LNKJYQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

boutell commented 2 years ago

searchable is mostly about opting out as I recall.

On Thu, Dec 2, 2021 at 1:54 PM Tom Boutell @.***> wrote:

Ah, no, searchable and silent should be expected to work, although I haven't reviewed the implementation.

On Thu, Dec 2, 2021 at 1:15 PM Alex Bea @.***> wrote:

  • The task node app @apostrophecms/search:index fails with TypeError: self.indexDoc is not a function
  • Pieces types are not registered programmatically as stated in the source documentation even with explicit searchable: true settings.

I've resolve those two in an upcoming PR.

@boutell https://github.com/boutell Were you saying that searchable (and silent maybe as well) should not be expected to operate as the code and code comments suggest? They're deprecated A2 features? I'm finding the same thing as @myovchev https://github.com/myovchev. Probably the same question for addSearchTexts since it's in the code, though I haven't tested that yet.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe/issues/3193#issuecomment-984876939, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27OKCSKN76YTI3USPE3UO6ZSNANCNFSM47LNKJYQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

boutell commented 2 years ago

I don't know the cause of searchable and silent not working for string fields, areas, etc. as expected, but they should work and I do consider it a bug if they don't. Hoping that was the question.

myovchev commented 2 years ago

@boutell I'm not sure how it's related, but there is also (maybe legacy) inconsistency I've found while digging around to find a way to add high search text to an article coming from related tags titles. My obvious choice was an event, so I've found doc-type listening to this one ('@apostrophecms/search:index'), which is exactly what I need: https://github.com/apostrophecms/apostrophe/blob/7d728dd4b8443a6649c30e1f33abf7a3dc08f9e5/modules/%40apostrophecms/doc-type/index.js#L222 The thing is this event isn't triggered at all - there is no trace of emit('index') in the search module. Furthermore, search module is directly hooking into doc-type via beforeSave method (which is the problematic function indexDoc() which should become a method because it's used by the index task): https://github.com/apostrophecms/apostrophe/blob/7d728dd4b8443a6649c30e1f33abf7a3dc08f9e5/modules/%40apostrophecms/search/index.js#L100

The summary - currently only the beforeSave event handler of the search module is executed, and the index event listener in doc-type is not. The latter I think is responsible for registering the index data from the doc fields via the schema manager.

abea commented 2 years ago

'@apostrophecms/search:index' matches an A2 docSearchIndex event that looks left out of getSearchTexts in A3. Maybe that simply needs to be added back.

@myovchev The indexDoc issue should be resolved in tomorrow's release through my PR #3573

myovchev commented 2 years ago

Interesting enough, my second thought was to extend (improve) getSearchTexts in order to add my precious tag titles to the article search index :) It seems if this is a desired behavior of the search index event, the solution is simple. I'll just wait for a repair :)