FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat: validate file formats in url (#1606) by jupyterjazz · Pull Request #1669 · docarray/docarray · GitHub

/ docarray Public

feat: validate file formats in url (#1606) - #1669

Merged
JoanFM merged 24 commits into
mainfrom
feat-file-validation
Jun 27, 2023
ConversationCommits (24)ChecksFiles changed
Merged

feat: validate file formats in url (#1606)#1669
JoanFM merged 24 commits into
mainfrom
feat-file-validation

Conversation

jupyterjazz commented Jun 26, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Followup of #1606

Approach:

  1. Validate given url based on what type mimetypes will guess
  2. If the first step is not successful, try validating against extra extensions provided for each url type

Why was CI failing in Kalim's PR:
Apparently mimetypes additionally uses system's mime.types file which is unique for different operating systems, even for different versions of the same operating system. Because of this file, mimetypes was guessing different types locally and on CI, resulting in strange errors. I disabled it by mimetypes.init([]) which means mimetypes will ignore system's mime.types and return same types every time

feat: validate file formats in url (#1606)
49fd592 
Signed-off-by: Mohammad Kalim Akram <kalim.akram@jina.ai>
test: reverting some changes
5680e0d 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>
chore: add prints
0ce7e54 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>
style: run black
c1e1528 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>
chore: print values
5591d3e 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>
feat: initialize mime types
d4a289f 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>
github-actions Bot added size/xl and removed size/m labels Jun 26, 2023

github-actions Bot commented Jun 26, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

refactor: file name
a69c197 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>

github-actions Bot commented Jun 26, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

refactor: rename again
fa0dbb2 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>

github-actions Bot commented Jun 26, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

JoanFM requested changes Jun 26, 2023
View reviewed changes
Comment thread docarray/typing/url/text_url.py Outdated
refactor: remove special cases
d438dc6 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>

github-actions Bot commented Jun 26, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

test: resolve some tests
d0948fc 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>

github-actions Bot commented Jun 26, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

refactor: remove custom mimetypes
5222026 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>
github-actions Bot added size/m and removed size/xl labels Jun 26, 2023
test: add a valid link
08c8b3b 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>
refactor: just want to make ci green am i asking too much?
262190f 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>
refactor: validate approach, should fail
c661282 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>
refactor: text link
7644d6d 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>
jupyterjazz marked this pull request as draft June 26, 2023 15:22
test: resolve tests
fbe7d7c 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>
refactor: polish up the code
4ee48f3 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>
style: run black
1a69277 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>
jupyterjazz marked this pull request as ready for review June 26, 2023 21:22
jupyterjazz requested a review from JoanFM June 26, 2023 21:22
JoanFM requested changes Jun 26, 2023
View reviewed changes
Comment thread docarray/typing/url/any_url.py
Comment thread docarray/typing/url/audio_url.py Outdated
Comment thread docarray/typing/url/text_url.py Outdated
Comment thread docarray/typing/url/url_3d/url_3d.py Outdated

@classmethod
def mime_type(cls) -> str:
return 'application'

JoanFM Jun 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

what is this mime type? use constants alsl

jupyterjazz Jun 26, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is a broad category for mimetypes. Contains obj, pdf, json, xml and many other files.. Now that I think about it, we should make it more specific (whatever is associated with .obj extension because that's what we usually use) and for other non-obj files rely on extra extensions. Changed accordingly.

Comment thread tests/integrations/predefined_document/test_audio.py
str(TOYDATA_DIR / 'hello.ogg'),
str(TOYDATA_DIR / 'hello.wma'),
str(TOYDATA_DIR / 'hello.aac'),
str(TOYDATA_DIR / 'hello'),

JoanFM Jun 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

why removed?

jupyterjazz Jun 26, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Because it's an Audio URL without an audio extension and should not be validated

JohannesMessner Jun 27, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I am not sure about that, (at least on unix) i can store an audio file without extension I believe, why should that not be allowed? Admittedly, for audio it may not be common to do that, but text files do it, e.g. Dockerfile

jupyterjazz Jun 27, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes you can, but in order to avoid issues like #1555 we need to look at extensions.

But you have a good point, text files without extensions are very common. Does it make sense to ignore validating text URLs that have no extensions? I don't really have another solution, we can't guess extensions or types in that scenario, and trying to read them during validation will be slow

JoanFM Jun 27, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

let's ignore validation of TextURL then?

JohannesMessner Jun 27, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can't we have a rule that is like "if there is an extension, validate it; if there is no extension, pass validation"? We could have that for all url types, no?

jupyterjazz Jun 27, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

"if there is an extension, validate it; if there is no extension, pass validation"

yeap this is what I meant, but for text urls only.
ok let's do it for all urls

Comment thread tests/integrations/predefined_document/test_audio.py
Comment thread tests/units/typing/url/test_text_url.py Outdated
refactor: add constants, update 3d mimetype
02c15c6 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>
test: resolve tests
32cc3ab 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>
refactor: remove prints
730f21b 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>
jupyterjazz linked an issue Jun 27, 2023 that may be closed by this pull request
Url types are not aware of extension during validation #1555
Closed
JohannesMessner reviewed Jun 27, 2023
View reviewed changes
Comment thread docarray/typing/url/any_url.py Outdated
Comment on lines +73 to +74
filename = url_parts[0].split('.')
extension = filename[-1] if len(filename) > 1 else None

JohannesMessner Jun 27, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

maybe I am being overly cautious here, but do we know for a fact that there are no corner cases where this splitting into filename and extension could break? Is there some resource or standard that we can reference?
Alternatively, I think pydantic implements some of this internally, Maybe we could repurpose some of their logic?

jupyterjazz Jun 27, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yeah there are many edge cases indeed. I already changed that part, can you take a look again? here are unit tests
https://github.com/docarray/docarray/pull/1669/files#diff-f1502e8b25d6058d51f22b4de5d853aeba8e107952a8b597848f8a918cb055fd

I'll explore how pydantic's doing that

jupyterjazz Jun 27, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

but I think this is ok for now, wdyt?

feat: pass validation for urls with not ext
9464fb7 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>
refactor: get ext
ee29f6a 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>
test: resolve unit tests
70d4970 
Signed-off-by: jupyterjazz <saba.sturua@jina.ai>

github-actions Bot commented Jun 27, 2023

Copy link
Copy Markdown

📝 Docs are deployed on https://ft-feat-file-validation--jina-docs.netlify.app 🎉

JoanFM merged commit e0e5cd8 into main Jun 27, 2023
JoanFM deleted the feat-file-validation branch June 27, 2023 14:02
JoanFM mentioned this pull request Jul 3, 2023
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

JoanFM JoanFM approved these changes
JohannesMessner JohannesMessner approved these changes

Assignees

No one assigned

Labels

area/core area/testing area/typing size/m

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Url types are not aware of extension during validation

4 participants

Back | FazBrowse Home | New Git URL