Skip to content
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

feat(input_schema): Resource properties #484

Merged
merged 5 commits into from
Oct 30, 2024

Conversation

MFori
Copy link
Contributor

@MFori MFori commented Oct 22, 2024

Update of input schema based on apify/apify-docs#1261:
New properties in schema: resourceProperty and resourceArrayProperty
To distinguish between resource properties with resourceType and string/array properties custom validation is done in validateField (because of clash on same type)

Input ui to support this schmena is ready here: https://github.com/apify/apify-core/pull/18093

@MFori MFori added the t-console Issues with this label are in the ownership of the console team. label Oct 22, 2024
@MFori MFori self-assigned this Oct 22, 2024
@github-actions github-actions bot added this to the 101st sprint - Console team milestone Oct 22, 2024
@github-actions github-actions bot added the tested Temporary label used only programatically for some analytics. label Oct 24, 2024
@MFori MFori marked this pull request as ready for review October 24, 2024 22:01
Copy link
Member

@drobnikj drobnikj left a comment

Choose a reason for hiding this comment

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

Looks good, just have two idea on how we can simplify.

packages/input_schema/src/schema.json Outdated Show resolved Hide resolved
}
}
}
},
"additionalProperties": false,
"required": ["title", "type", "properties", "schemaVersion"],
"definitions": {
"resourceProperty": {
"title": "Resource property",
Copy link
Member

Choose a reason for hiding this comment

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

Nit: It looks the title, description, nullable, sectionCaption, sectionDescription appear in all definition. Could we move these to special definitions?

{
...
    "definitions": {
        "baseProperty": {
            "type": "object",
            "properties": {
                "title": {"type": "string"},
                "description": {
                    "type": "string"
                },
                "nullable": { "type": "boolean"}
            },
            "required": ["title", "description"]
        },
        "resourceProperty": {
            "allOf": [
                {"$ref": "#/definitions/baseProperty"},
                {
                    "properties": {
                        ...the rest of props on top of baseProperty
                    }
                }
            ],
            "required": ["type", "title", "description", "resourceType"]
        },
        ...
    }
...
}

Copy link
Member

@gippy gippy left a comment

Choose a reason for hiding this comment

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

I like the "allOf" base property Kuba has. Otherwise preapproving

@MFori
Copy link
Contributor Author

MFori commented Oct 29, 2024

The solution with baseProperty and allOf has some issues:

  • additionalProperties doesn't work for this, but yeah we can use unevaluatedProperties in this case
  • we are accessing field property.properties in our custom validation and this property wouldn't have this property and additional transformation will be needed to check if the property have some field or not and then merge properties from the allOf items.
  • the baseProperty is still validated and will be need to filter it out in our custom validation
  • lot of tests fails also on other issues then the points above and needs to be resolved

So it's possible but do we really need it in this case with the increased complexity of validation?
@gippy @drobnikj

According to: why not in condition? I prefer condition without not and switch else branches, it will make it more readable.
If I switch the else branches lot of our tests fails I am on it and trying to figure out what's going on there

@gippy
Copy link
Member

gippy commented Oct 29, 2024

The solution with baseProperty and allOf has some issues:

  • additionalProperties doesn't work for this, but yeah we can use unevaluatedProperties in this case
  • we are accessing field property.properties in our custom validation and this property wouldn't have this property and additional transformation will be needed to check if the property have this field or not and then merge properties from the allOf items.
  • the baseProperty is still validated and will be need to filter it out in our custom validation
  • lot of tests fails also on other issues then the points above and needs to be resolved

So it's possible but do we really need it in this case with the increased complexity of validation? @gippy @drobnikj

According to: why not in condition? I prefer condition without not and switch else branches, it will make it more readable. If I switch the else branches lot of our tests fails I am on it and trying to figure out what's going on there

I liked the idea because it reduces repetition of the schema, but if it's at the cost of increased complexity of the schema, then I do not mind the current solution where the properties are repeated.

@MFori
Copy link
Contributor Author

MFori commented Oct 29, 2024

Nit: why not in condition? I prefer condition without not and switch else branches, it will make it more readable.

@drobnikj I was able to remove this condition at all by updating our custom validation and moving some tests from input_schema_definition.test.ts to input_schema.test.ts because the second one uses our validation but the first one doesn't.

@MFori MFori requested a review from drobnikj October 29, 2024 10:16
packages/input_schema/src/types.ts Outdated Show resolved Hide resolved
@MFori MFori merged commit ccfca82 into master Oct 30, 2024
9 checks passed
@MFori MFori deleted the feat/input-schema-resource-properties branch October 30, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t-console Issues with this label are in the ownership of the console team. tested Temporary label used only programatically for some analytics.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants