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

fix(pipes): add parse-file pipe #8965

Closed
wants to merge 4 commits into from

Conversation

coolsamk7
Copy link

@coolsamk7 coolsamk7 commented Jan 19, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #4752

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@coveralls
Copy link

Pull Request Test Coverage Report for Build 76298ff5-c01e-441e-83fa-b6af4557d80f

  • 7 of 20 (35.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 93.933%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/common/pipes/parse-file.pipe.ts 4 17 23.53%
Totals Coverage Status
Change from base Build 030c7bfc-7fd6-457a-8c24-3f90c6cb9edf: -0.2%
Covered Lines: 5698
Relevant Lines: 6066

💛 - Coveralls

Copy link
Contributor

@thiagomini thiagomini left a comment

Choose a reason for hiding this comment

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

Some suggestions regarding the future implementations

packages/common/pipes/parse-file.pipe.ts Outdated Show resolved Hide resolved
Comment on lines +59 to +76
if (
validationOptions.maxFileSize &&
validationOptions.maxFileSize < object.size
) {
throw this.exceptionFactory(
`Validation failed (expected size is less ${validationOptions.maxFileSize})`,
);
}
if (
validationOptions.fileType &&
validationOptions.fileType !== object.mimetype
) {
throw this.exceptionFactory(
`Validation failed (expected file type is ${validationOptions.fileType})`,
);
}
return object;
}
Copy link
Contributor

@thiagomini thiagomini Jan 21, 2022

Choose a reason for hiding this comment

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

Altough this solution works for its intended purpose, I think it might be better to think of later needs for validation, or even consider the possibility of passing a custom validation function.

One suggestion is to handle it with Chain of Responsibility pattern, that way you achieve at same time the Open-Closed principle, allowing to extend the possible validations in the future, and also you could provide the user with the ability to define their own handlers to validate the file. I also like to implement chain of responsibility with a processor, which handles the execution of all handlers.

Example of handler interface:

export interface FileValidator {
    
   // This function determines if given handler should validate the incoming file. You could use it to check wether the option was given
   accepts(validationOptions: ValidationOptions): boolean
   
   // The return is a string because it is meant to be the reason of failure. if null, then validation passed
   validate(file: any, validationOptions: ValidationOptions): string
}

Example of handler:

export class FileSizeValidator implements FileValidator {
  accepts(validationOptions: ValidationOptions): boolean {
    return !!validationOptions.maxFileSize;
  }

  validate(file: any, validationOptions: ValidationOptions): any {
    if (file.size > validationOptions.maxFileSize) return `expected file type is ${validationOptions.fileType}`
  }

Example of processor:

export class FileValidationProcessor {
  constructor(private readonly validators: FileValidator[]) {
  }

  validateFile(file: any, validationOptions: ValidationOptions): string {
    const acceptingValidators = this.validators.filter(validator => validator.accepts(validationOptions))

    for (const validator of acceptingValidators) {
      const failedMessage = validator.validate(file, validationOptions);
      if (failedMessage) {
        return failedMessage
      }
    }
  }
}

Using the strategy above makes possible to add other validators without changing the already existing code (closed to changes).

@coolsamk7
Copy link
Author

I will work on it this weekend

@kamilmysliwiec
Copy link
Member

🏓

@thiagomini thiagomini mentioned this pull request May 31, 2022
12 tasks
@kamilmysliwiec
Copy link
Member

Let's track this here #9699

@coolsamk7 coolsamk7 deleted the file_validation_pipe branch June 30, 2022 14:41
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.

4 participants