Skip to content

Conversation

@pacyL2K19
Copy link
Contributor

@pacyL2K19 pacyL2K19 commented Nov 18, 2024

This PR

Ensures proper type association for nestedProperties in data type configurations

  • Fix Fix nested properties typing when "strict": false #203
  • Resolved incorrect handling of nestedProperties for non-object data types:
    • Previously, even primitive types like string or boolean could accept nestedProperties when strict mode was enabled, which is technically invalid.
    • When strict mode was disabled, valid cases involving objects also triggered errors.
  • Introduced ObjectDataType and PrimitiveDataType to improve type distinction and prevent misuse.
  • Updated NestedPropertyCreate and NestedPropertyConfigCreate to align with conditional dataType logic.
  • Preserved support for deeply nested properties with proper validation.

Current behavior (without the fix)

Strict Mode: true

Primitive types (string, boolean, etc.) erroneously accept nestedProperties, which is technically impossible.
Example (Invalid but currently allowed):

const invalidConfig = {
  name: 'Example',
  dataType: 'text', // Primitive type
  nestedProperties: [
    { name: 'Invalid', dataType: 'number' }, // ❌ should trigger a typescript error here, but doesn't
  ],
};

Strict Mode: false

Both invalid and valid configurations trigger errors.
For instance, even proper configurations like the following are flagged:

const validConfig = {
  name: 'ValidExample',
  dataType: 'object',
  nestedProperties: [
    { name: 'ValidNested', dataType: 'text' }, // should not trigger a typescript error but currently triggering `Type '{ name: string; dataType: "text"; }' is not assignable to type 'never'.` which is incorrect
  ],
};

Behavior With This Fix

Primitive Types:

Cannot accept nestedProperties regardless of whether strict mode is enabled or not(desired behavior).
Example (Now correctly flagged as invalid):

const invalidConfig = {
  name: 'Example',
  dataType: 'text',
  nestedProperties: [
    { name: 'Invalid', dataType: 'number' }, // ❌ triggering the `Type '{ name: string; dataType: "text"; }' is not assignable to type 'never'.` error preventing developers from trying this (desired behavior)
  ],
};

Object Types:

Continue to support deeply nested configurations without errors.
Example (Valid and supported):

export const configs: CollectionConfigCreate<undefined, string> = {
  name: 'Movies',
  properties: [
    {
      name: 'Testing',
      dataType: dataType.OBJECT_ARRAY,
      nestedProperties: [
        {
          name: 'width',
          dataType: dataType.NUMBER,
        },
        {
          name: 'isBool',
          dataType: dataType.BOOLEAN,
        },
        {
          name: 'object-with-nested',
          dataType: dataType.OBJECT,
          nestedProperties: [ // ✅ no matter the strict flag, works as expected -> expected behavior
            {
              name: 'nested-url',
              dataType: dataType.TEXT,
            },
            {
              name: 'another-array-object-in-nested',
              dataType: dataType.OBJECT_ARRAY,
              nestedProperties: [ // ✅ works as expected
                {
                  name: 'nested-url',
                  dataType: dataType.TEXT,
                },
              ],
            },
          ],
        },
      ],
    },
  ],
};

The updated types now prevent invalid configurations and allow valid nested configurations without breaking existing functionality no matter the strict flag in tsconfig

@pacyL2K19 pacyL2K19 changed the title fix: Improve nestedProperties' type configuration Nov 18, 2024
@weaviate-git-bot
Copy link

To avoid any confusion in the future about your contribution to Weaviate, we work with a Contributor License Agreement. If you agree, you can simply add a comment to this PR that you agree with the CLA so that we can merge.

beep boop - the Weaviate bot 👋🤖

PS:
Are you already a member of the Weaviate Slack channel?

@pacyL2K19
Copy link
Contributor Author

I agree with the CLA

@tsmith023
Copy link
Collaborator

Hey @pacyL2K19, thank you so much for this great contribution! I must admit, when looking into this I didn't have a great solution for it so I'm glad you've found this!

The failing tests are due to some new behaviour on WCD, which we use for one journey test. I just pushed a fix for that so feel free to rebase on main and then your tests should be green, cheers!

… configuration

- Resolved incorrect handling of `nestedProperties` for non-object data types.
- Introduced `ObjectDataType` and `PrimitiveDataType` for improved type distinction.
- Updated `NestedPropertyCreate` and `NestedPropertyConfigCreate` to align with conditional `dataType` logic.
- Improved type safety and consistency in data type configurations.
@pacyL2K19 pacyL2K19 force-pushed the bugfix/#203-nested-properties branch from f6ffc07 to 1bfedd7 Compare November 18, 2024 10:22
@pacyL2K19
Copy link
Contributor Author

Hey @pacyL2K19, thank you so much for this great contribution! I must admit, when looking into this I didn't have a great solution for it so I'm glad you've found this!

The failing tests are due to some new behaviour on WCD, which we use for one journey test. I just pushed a fix for that so feel free to rebase on main and then your tests should be green, cheers!

Thank you so much @tsmith023
It's my pleasure
Just rebased with the main branch 🤞🏾

Copy link
Collaborator

@tsmith023 tsmith023 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks again for the contribution! ❤️

@tsmith023 tsmith023 merged commit acf2462 into weaviate:main Nov 21, 2024
8 checks passed
@tsmith023
Copy link
Collaborator

Ah, I tried it out locally now that it's merged and still observe the same problem reported in the issue when strict: false. However, I think there's an easy fix for the issue that I missed in the review: #233. LMK if you think this change is good from your PoV! @pacyL2K19

@pacyL2K19
Copy link
Contributor Author

Ah, I tried it out locally now that it's merged and still observe the same problem reported in the issue when strict: false. However, I think there's an easy fix for the issue that I missed in the review: #233. LMK if you think this change is good from your PoV! @pacyL2K19

Hi @tsmith023
I tried locally the types and they seemed okay, let me have another look at it now

@tsmith023
Copy link
Collaborator

tsmith023 commented Nov 21, 2024

For clarity, this tsconfig.json file:

{
  "compilerOptions": {
    "target": "ESNext",
    "module": "NodeNext",
    "moduleResolution": "NodeNext",
    "esModuleInterop": true,
    "skipLibCheck": true,
    "allowSyntheticDefaultImports": true,
    "strict": false,
  },
}

and this code:

import { CollectionConfigCreate, dataType } from "weaviate-client";

function func(): CollectionConfigCreate {
  return {
    name: "Name",
    properties: [
      {
        name: 'images',
        dataType: 'object[]',
        nestedProperties: [
          {
            name: 'url',
            dataType: dataType.TEXT,
          },
          {
            name: 'caption',
            dataType: dataType.TEXT,
          },
          {
            name: 'number',
            dataType: dataType.NUMBER,
          }
        ],
      },
    ],
  };
}

gives me compilation errors seen in the issue with the changes on main. Such errors are fixed by my PR 😁

@pacyL2K19 pacyL2K19 deleted the bugfix/#203-nested-properties branch November 22, 2024 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix nested properties typing when "strict": false

3 participants