Initial non searchable fields support on missing schema/doc key match

[LBRDan]

Hello there!
Thanks for this amazing piece of open source software! It should be an example for everyone out there as it is for me ❤️

First of all, let me apologize for being so rushy in opening this PR as a draft, but I'm currently running out of battery (hopefully be able to work again on this tonight or tomorrow (CET))
This PR contains a first implementation attempt for #171 issue. I wonder if this is the desired way of implementing this feature, it would be awesome to hear feedback from you

Thanks!

--- Edit:

This PR tries to implement the #171 request for non-searchable fields when they are included inside a document but they're not included inside the schema definition
I tried to implement the feature by:

• Skipping the schema - doc property check adherence inside the recursiveCheckDocSchema function
• Instructing recursiveTrieInsertion to be schema-aware when including a document field as searchable by traversing the doc and the schema at the same pace

[micheleriva]

Hey hey thank you so much for opening this!
Looking forward to see the PR completed 🙂

Let me know if you need any help at any point!

[LBRDan]

Let me know if you need any help at any point!

Thanks @micheleriva ❤️
The implementation is pretty much done... but I think that I'm missing something relative to the nested schema properties
I hope that it's only me not getting it right...

Basically, I don't think that recursiveCheckDocSchema is well... actually checking for schema adherence for nested schema/doc properties 😅
Let me use an example trying to explain this better (using some docs found in the tests)

ATM assertDocSchema uses recursiveCheckDocSchema to check the doc (or docs[]) against the schema.
https://github.com/LyraSearch/lyra/blob/cc94cf31253168600407b8088f29af0bda45a1a5/src/lyra.ts#L289

While I was expecting that recursiveCheckDocSchema was recursively called returning the value of its computation, I found out that using a document (and a schema) with nested fields, the properties of nested properties bypass the checking as nothing gets properly returned from the recursive self call (line 199 below)

https://github.com/LyraSearch/lyra/blob/cc94cf31253168600407b8088f29af0bda45a1a5/src/lyra.ts#L187-L206

Something like the following crashes recursiveTrieInsertion in the current main branch, but recursiveCheckDocSchema should throw before that: (btw static type checking works correctly there)

    const db = create({
      schema: {
        quote: "string",
        author: {
          name: "string",
          surname: "string",
        },
        tag: {
          name: "string",
          description: "string",
        },
      },
    });

    insert(db, {
      quote: "Be yourself; everyone else is already taken.",
      author: {
        name: "Oscar",
        surname: "Wild",
      },
      tag: {
        name: "inspirational",
        description: "Inspirational quotes",
        // @ts-expect-error test error case
        cadrega: "so... the apple is the cadrega",
      },
    });

However, this other doc that have the same problem but assertDocSchema throws as expected:

    const db = create({
      schema: {
        quote: "string",
        author: {
          name: "string",
          surname: "string",
        },
        tag: {
          name: "string",
          description: "string",
        },
      },
    });

    insert(db, {
      quote: "A room without books is like a body without a soul.",
      author: {
        name: "Marcus",
        surname: "Tullius Cicero",
      },
      // @ts-expect-error test error case
      furio: "Madga, do you adore me? Then you see it's a mutual thing, do ya?",
      tag: {
        name: "books",
        description: "Quotes about books",
      },
    });

The same applies on wrong supported scalar schema types and doc type check

Is this intended to work like this?
If not, after this feature, the nested doc.tag.cadrega key should not crash the thing and should throw via assertDocSchema, but I'll need to change something more

It's not strictly related to this feature but (if you confirm my doubt) it could prevent the way I tried to implement this from working as expected

Also, if all of that turns out to be something real, the interface for the document insert function (to support both doc or doc[]) maybe needs to be slightly modified because it does not distinguish from nested object type doc property and "the first iteration for an array of docs", passing recursively the root schema when it's needed to pass the nested property schema

I really hope to be wrong and that I'm missing something 🤞 😓

[micheleriva]

Hey @LBRDan thank you so much for the analysis!

So, as for the current version of Lyra (v0.2.8 at the time of writing) recursiveCheckDocSchema should throw an error if there's a nested type/property that does not respect the schema.

For instance, given:

import { create, insert } from '@lyrasearch/lyra'

const db = create({
  quote: 'string'
  meta: {
    rating: 'number'
  }
})

this should throw:

insert({
  quote: 'foo bar baz',
  meta: {
    rating: 10,
    randomProperty: 'foo' // <-- this should throw
  }
})

This is certainly a bug and I'd kindly ask you to open a new issue for it, we can fix it separately after merging this PR (it will be released in v0.3.0 with #171.

Regarding this PR, I'd ask you to add more test cases, also with deeply nested properties and different types.

Thanks a lot!

[micheleriva]

Hi @LBRDan!
Do you have any idea when this PR will be ready for review? No hurry, just planning the next release

[LBRDan]

Hi @LBRDan! Do you have any idea when this PR will be ready for review? No hurry, just planning the next release

Sorry @micheleriva , I'm pushing the tests you requested just now and opening the PR to reviews!
I hope that everything is as you expect it 🤞

Anyway, I was wondering if a normal user should be able to enhance the schema (preventing type checking errors) via TS module augmentation (or something else), or we should also change the types like this (adding { [key: string]: unknown } for unmatched schema keys):

import type { PropertiesSchema } from "./lyra";
export type Nullable<T> = T | null;

type ResolveTypes<TType> = TType extends "string"
  ? string
  : TType extends "boolean"
  ? boolean
  : TType extends "number"
  ? number
  : TType extends PropertiesSchema
  ? { [P in keyof TType]: ResolveTypes<TType[P]> } & { [key: string]: unknown }
  : never;

export type ResolveSchema<T extends PropertiesSchema> = {
  [P in keyof T]: ResolveTypes<T[P]>;
} & { [key: string]: unknown };

export type SearchProperties<
  TSchema extends PropertiesSchema,
  TKey extends keyof TSchema = keyof TSchema,
> = TKey extends string
  ? TSchema[TKey] extends PropertiesSchema
    ? `${TKey}.${SearchProperties<TSchema[TKey]>}`
    : TKey
  : never;

What do you think about this?

Also, it took me a while to merge the frequency feature that landed on master. Some of the tests that were marked there as to skip, currently do not pass in my branch and I believe they should... Let me know if something slipped unnoticed after the conflict resolution from the merge

[micheleriva]

@LBRDan I just tested this locally, and it seems to work great! About the typings, we could consider it as a nice-to-have for a future release. Thank you so much for your contribution