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: implement File type #1017

Closed
wants to merge 4 commits into from

Conversation

jgradzki
Copy link

@jgradzki jgradzki commented Mar 14, 2022

#814

z.file().type(["image/png", "image/jpeg"]).min(2000).max(4000);
z.file().type("image/png");

For Node.js:

import { File } from "@web-std/file";

z.file({}, File);

Getters:

z.file().min(1).minSize;
z.file().max(5).maxSize;
z.file().type("image/png").allowedTypes

src/types.ts Outdated

static create = (): ZodFile => {
if (typeof File === "undefined") {
throw new Error("File not supported in current environment");

Choose a reason for hiding this comment

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

We could use something like @web-std/file

@jgradzki jgradzki force-pushed the feature/file-type branch 4 times, most recently from 126f2fb to 4682116 Compare March 14, 2022 12:35
@jgradzki
Copy link
Author

@JacobWeisenburger ☝️

src/types.ts Outdated Show resolved Hide resolved
src/types.ts Outdated
}

static create = <T extends Function = typeof File>(
fileConstructor?: T

Choose a reason for hiding this comment

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

If we want to support custom errors I think you need to pass RawCreateParams like in ZodString right?

Choose a reason for hiding this comment

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

I'm talking about this:
image

Choose a reason for hiding this comment

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

I think is the zod way to allow translated errors

Choose a reason for hiding this comment

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

I'm playing with your PR in my code. Looking nice at least the errors.
image

I'll check how it feels the validations : )

Copy link
Author

Choose a reason for hiding this comment

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

Ok, so no problem? :D
I have removed some extra " in "File not supported ..." error message, also changed separator in issue.expected.join from , to or

Copy link
Author

Choose a reason for hiding this comment

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

nvm, i see what you talking about, will update

Copy link
Author

Choose a reason for hiding this comment

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

i wonder which parameter should be first, for now file constructor is second

@jgradzki jgradzki force-pushed the feature/file-type branch 3 times, most recently from 3f7d096 to 97471b4 Compare March 14, 2022 14:57
@andresgutgon
Copy link

andresgutgon commented Mar 14, 2022

Ok, this is my real problem. Let's see if you have an idea 😄

When a Node request occur. Let's say a POST request multipart the user submit a form with text fields and file fields.

Then the request is processed and and validated. In the case of files what I had seen in other codes is that they write into Disk the data of that file and chunk by chunk they check if the current size of the file is lower than maxBytes.

If it's bigger they throw an Error and stop the stream.

I put a piece of code here that is kind of what's I'm talking

Now my question is how I use z.file(File).size and this flow?


static create = <T extends Function = typeof File>(
params?: RawCreateParams,
fileConstructor?: T

Choose a reason for hiding this comment

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

I would switch the order here. It's more common to set the constructor than the params I think. Feel free to ignore, just an opinion

Copy link
Author

Choose a reason for hiding this comment

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

maybe, I will wait for one more opinion maybe and then we will decide

package.json Outdated Show resolved Hide resolved
@jgradzki
Copy link
Author

Ok, this is my real problem. Let's see if you have an idea smile

When a Node request occur. Let's say a POST request multipart the user submit a form with text fields and file fields.

Then the request is processed and and validated. In the case of files what I had seen in other codes is that they write into Disk the data of that file and chunk by chunk they check if the current size of the file is lower than maxBytes.

If it's bigger they throw an Error and stop the stream.

I put a piece of code here that is kind of what's I'm talking

Now my question is how I use z.file(File).size and this flow?

Unfortunately I don't have an answer, I would have to dive in and research about it but I don't have that much time right now :(

@andresgutgon
Copy link

It would be nice to have a getter like string isEmail to get the expected types, min and max.

Would be something like this:

const myFile = z.file().min(2).max(39).type(["application/pdf", "image/png"])

// The getters
myFile.type // ["application/pdf", "image/png"]]
myFile.min // 2
myFile.max // 39

I would need this to use in a code like this one

@jgradzki
Copy link
Author

It would be nice to have a getter like string isEmail to get the expected types, min and max.

Would be something like this:

const myFile = z.file().min(2).max(39).type(["application/pdf", "image/png"])

// The getters
myFile.type // ["application/pdf", "image/png"]]
myFile.min // 2
myFile.max // 39

I would need this to use in a code like this one

added, had to use different names ;)

@andresgutgon
Copy link

andresgutgon commented Mar 15, 2022

Ok I think current implementation is flexible enough. For my use case I don't want to build a real File object while doing validation. But this code allow me to pass a class that looks like a File

export class FileShape { constructor(public type: string, public size: number) {}}

// Then I can use normal:
const myFile = z.file({}, FileShape).max(34033).type(["images/jpeg"])

The reason for not wanting to use a real File implementation is because I don't know the size of the file in advance on the server. So I can't build a real file and pass to zod.
What I do is to stream the contents on the file + write to disk those contents and measure if they are less than the .max attribute on the zod file validation.

When I know that the size of the file submitted is too big I set that size in my ShapeFile like this

const file = new FileShape("image/jpeg", 0)
const myValidation = z.file({}, FileShape).max(34033).type(["images/jpeg"])
// ...
// Do some work with Node streams + write to disk + measure bytes
// If I detect the the file is too big set that size in my FileShape like this:
file.setSize(23232323) // Too big
myValidation.parse(file) // Now that I know the actual file size I want to get the errors.

Do you thing is a reasonable way of using this or it's fragile?

I like your PR 👍

message = `The file type should be ${issue.expected.join(
" or "
)}. Received: ${issue.received}`;
break;

Choose a reason for hiding this comment

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

Missing here messages for too_big and too_small. Without it for file the error goes to the generic:
image

Copy link

Choose a reason for hiding this comment

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

I can do these changes if you give me push permission to your branch. If not is fine I'll wait :)

}

export class ZodFile<FileConstructor extends Function> extends ZodType<
FileConstructor,

Choose a reason for hiding this comment

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

Would be possible to support an Array of Files?

The use case is if people use multiple=true in an <input type="file" multiple />

This way these upload can be processed in the server and use this validation like this:

const myRequest = z.object({
  gallery: z.file({}, File).array()
})

But for this to work we need to manage the case when the input is an array.

I would play with it in my dev environment with your fork : )

Choose a reason for hiding this comment

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

Forget it. This works:

const myRequest = z.object({
  gallery: z.array(z.file({}, File))
})

Nothing to do here : )

@andresgutgon
Copy link

@jgradzki could you take a look to my comments? I would really love to have this in "zod" ❤️

@colinhacks what's your opinion here?

@andresgutgon
Copy link

I'm being thinking on the approach we're taking here of requiring a constructor. It has some issues.

The first issue is that you need to pass a constructor on Non-web environments or simulate a web file API in Node. This is annoying and I think we can avoid it.

The second issue is that what you see after parsing your ZodFile is typeof File
image

This is a problem because then Typescript doesn't know what properties should have the returned File | FileLike class.

My proposal

My proposal is to define a minimum common interface that ZodFile should require and any class that respect this interface can be returned.

interface IFile {
  name: string;
  size: number;
  type: string;
}

If we return an Interface then Typescript know what is that. Look how nice : )

image

Also here is clear what you have
image

Implementation

I'm still working on it. That interface is missing for example filename which is basic in a node environment. Also we should add all official web attributes and methods according to the spec

But not check that passed object is an instanceof File. Instead just check that it has what we need for this ZodFile which is size and type

    const hasSize = !!input.data?.size;
    const hasType = !!input.data?.type;
    if (!hasSize || !hasType) {
      const issue = makeIssue({ //... set the issue as you do })
    }

What do you think?

@andresgutgon
Copy link

andresgutgon commented Mar 25, 2022

Please take a look to THIS SANDBOX I put together both apprach fileInterface.ts and fileConstructor.ts

image

> dateSchema.safeParse("2022-01-12T00:00:00.000Z"); // success: true
> ```

## Files [browser only]

Choose a reason for hiding this comment

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

Why only browser? At this point a lot of people is using Zod in browser and in Node. Why not a solution valid for both environments?

In my case it looks something like this:

const schema = z.object({
  name: z.string(),
  avatar: z.file() 
})

const requestHandler = (request, response) => {
  const result = parser.parse({ request, schema })

  if (!result.success) {
     // Handle error
  } 
  
  // At this point is a file in NodeJS. That can be a file in filesystem o an object in S3
  console.log("DATA", result.data.avatar)
}

All the implementation on how to create that file is inside the parser. But Zod is there to provide a nice validation system. Would be super nice if instead of file type be limited to a web File object would be a contract that can satisfy both web and Node.

I'm not thinking on adding any Node dependency here. Just have a file validator that accept a contract instead of a fixed File class. In the end the validator just want the file to respond to size and type properties right?

Copy link
Author

Choose a reason for hiding this comment

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

I started thinking that we cannot satisfy browser and node.js in one type. My intentions were strictly to validate for File instance. I think there should be completely different type for node.js/deno and I think you should make your own in your project as handling files on server can be done in few different ways I guess.

Copy link

Choose a reason for hiding this comment

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

I started thinking that we cannot satisfy browser and node.js in one type. My intentions were strictly to validate for File instance. I think there should be completely different type for node.js/deno and I think you should make your own in your project as handling files on server can be done in few different ways I guess.

That's the thing : )

  1. In browser a File is checked for size and type
  2. In Node/Deno a Something is checked for size and type

The only job of this validator is to check if the Thing passed respond to type and size

And then in the return part could be something like this:

interface IFile {
  public name: string
  public size: number
  public filepath: string
}

As inspiration this what AdonisJS does for handling Disk, S3, GCP, Azure implementations. That is their common File interface. I understand maybe this is not Zod business so please feel free to ignore. I just want to expose what I'm seing in the Node side and what would be the best way to integrate Zod as a validation layer for a Node request.

I like about this proposal that is flexible enough to serve browser and Node.

Cons

In browser you would have to wrap your File with an IFile like interface

class WebFile implements IFile {
  filepath: string = ""
  constructor(file: File) {
     this.name = file.name
     this.size = file.size
  }
}

Choose a reason for hiding this comment

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

Totally fine with implementing my own file for Node 😄

But I think Zod is awesome and people is already using it in the server. file should handle both platforms somehow. But as I said totally happy if this doesn't happen inside Zod

// type DateSchema = Date
z.date().min(1); // no empty files
z.date().max(10e6); // max 10MB
z.date().types(["image/png", "image/jpeg"]);
Copy link
Author

Choose a reason for hiding this comment

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

z.file I guess :D

Copy link
Author

Choose a reason for hiding this comment

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

@colinhacks what next :)

@stale
Copy link

stale bot commented Jul 20, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Jul 20, 2022
@stale stale bot closed this Jul 27, 2022
@ricardo-valero
Copy link

What happened to this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants