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

Enable Dynamic Schema Support for File Type Definitions in Analysis Types #858

Merged
merged 12 commits into from
Sep 30, 2024

Conversation

Azher2Ali
Copy link
Contributor

@Azher2Ali Azher2Ali commented Sep 19, 2024

  1. Base Schema Updates:

    • fileType is now its own definition, allowing any string value.
    • file.fileType references this new definition, ensuring that it is always a required property for files.
  2. Analysis Type Registration Schema:

    • If a user provides a file.fileType definition, it merges with the base schema definition.
    • Users can now add additional restrictions on file.fileType within the dynamic schema.
  3. Data Structure Enhancements:

    • Added support for an optional options.fileTypes array in the analysisType dynamic schema, allowing users to define specific allowed file types.
    • Updated the controller and analysisTypeService.register to accept the options object.
  4. Validation Enforcement:

    • The analysis submission/update process has been updated to enforce allowed fileTypes from the schema, in addition to JSON Schema validation.
    • If no file types are defined in the dynamic schema, all file types are accepted.

Notes:

  • Additional validation logic has been implemented to ensure that the file types of each file submitted in an analysis match the allowed types defined in the dynamic schema, if provided.
  • If no specific file types are defined for an analysis type, the check is skipped.

Impact:

This change provides a more flexible and dynamic way to handle file type validations for various analysis types, paving the way for future enhancements and custom analysis schemas.

@Azher2Ali Azher2Ali changed the title Feature/test changes Enable Dynamic Schema Support for File Type Definitions in Analysis Types Sep 23, 2024
@Azher2Ali Azher2Ali requested a review from joneubank September 25, 2024 19:15
Copy link
Contributor

@joneubank joneubank left a comment

Choose a reason for hiding this comment

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

This looks complete, just two small nitpicks.

Can you add a comment to this PR that describes the behaviour when we are creating a new version of an existing schema - if you don't specify file types does it keep the existing file type restrictions or remove the existing restrictions? We can create another ticket to update this behaviour if it is unintuitive, I just want to record what the behaviour is.

.fileTypes(fileTypes)
.build();

log.info("file types " + fileTypes);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like developer test logs, I think we can remove this or change it to debug level and add more details from the analysis schema that is being created.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -849,7 +850,7 @@ public static List<AnalysisType> generateData(
i -> {
val name = names.get(i % repeats);
val schema = generateRandomRegistrationPayload(randomGenerator);
val out = analysisTypeService.register(name, schema);
val out = analysisTypeService.register(name,new AnalysisTypeOptions(), schema);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like code formatting hasn't been run on this line, no space between comma and next argument.

@Azher2Ali Azher2Ali requested a review from joneubank September 30, 2024 14:13
Copy link
Contributor

@joneubank joneubank left a comment

Choose a reason for hiding this comment

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

Looking good!

@Azher2Ali Azher2Ali merged commit cded800 into develop Sep 30, 2024
2 checks passed
@Azher2Ali Azher2Ali deleted the feature/test_changes branch September 30, 2024 14:48
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.

2 participants