Addons-server: Refactor `get_mime_type_for_blob()`

Created on 30 Apr 2020  路  9Comments  路  Source: mozilla/addons-server

The get_mime_type_for_blob function does content sniffing and file extension-based look-up to assign a mimetype to a given file: https://github.com/mozilla/addons-server/blob/1d76cfa07d7f03ce06dbe2465e06acdf9bc70d48/src/olympia/lib/git.py#L108-L158

It currently has 3 ways of finding a mimetype:

  1. a small map named SIMPLIFIED_MIMETYPE_DETECTION
  2. content sniffing with the magic lib (libmagic1)
  3. it tries to find a more specific mimetype using the mimetypes lib (which uses some system files like /etc/mime.types)

That seems overly complicated and I am not quite sure to understand why we have 3 different techniques (although (2) and (3) combined makes perfectly sense).

I suggest we simplify this function by:

  1. removing SIMPLIFIED_MIMETYPE_DETECTION
  2. removing the code that uses magic, including the code to install libmagic1 in the docker and travis-ci envs
  3. relying on mimetypes.guess_type() only, which uses a system file as source. This would mean putting a mime.types file under version control (which would be copied during the docker build and on travis-ci as well).

Note: we updated the /etc/mime.types file on travis-ci to be similar to the docker image one in https://github.com/mozilla/addons-server/issues/14072


For QA: syntax highlighting in code-manager should work as before on common files.

code manager git p3 verified fixed

All 9 comments

@diox WDYT? Any concerns? The mimetype value is only used by code-manager to configure syntax highlighting and we do have this SIMPLIFIED_MIMETYPE_DETECTION map so I doubt we want to be hyper-accurate.

That sounds fine. I also don't think we need super accurate mime type handling for code-manager, something fast based on filename sounds good.

@willdurand I verified on AMO dev with FF76(Win10).

Compared with the previous behavior the policy of disabling the syntax highlighting did not change because I've been testing with the same files on AMO stage and got the same results.

1. All Browse pages have the highlighting enabled.

2. Compare pages have both:

  • syntax is enabled for files with code lines (without minified content or long blocks of code on a single line).

Examples:
https://code.addons-dev.allizom.org/en-US/compare/618525/versions/1689955...1689956/?allowSlowPages=false&path=src%2Foptions%2Fstyle.bundle.css 25 lines of code
https://code.addons-dev.allizom.org/en-US/compare/619691/versions/1691089...1691184/?allowSlowPages=true&path=ui%2Fpopup%2Fcompatibility.js - 39 lines
https://code.addons-dev.allizom.org/en-US/compare/619691/versions/1691089...1691184/?allowSlowPages=true&path=ui%2Fdevtools%2Findex.html - 44 lines
https://code.addons-dev.allizom.org/en-US/compare/618525/versions/1689955...1689956/?allowSlowPages=false&path=src%2Fcontribute%2Fstyle.bundle.css - 97 lines

https://code.addons-dev.allizom.org/en-US/compare/619691/versions/1691089...1691184/?allowSlowPages=true&path=ui%2Fstylesheet-editor%2Findex.js - 1447 lines
https://code.addons-dev.allizom.org/en-US/compare/619691/versions/1691089...1691184/?allowSlowPages=true&path=ui%2Fdevtools%2Findex.js - 1469 lines
https://code.addons-dev.allizom.org/en-US/compare/619691/versions/1691089...1691184/?allowSlowPages=true&path=ui%2Fdevtools%2Fstyle.css - 851 lines

Meanwhile for .json files of 156 code lines, the highlighting was disabled:

https://code.addons-dev.allizom.org/en-US/compare/619691/versions/1691091...1691184/?allowSlowPages=false&path=_locales%2Fbe%2Fmessages.json

So I can't explain it why or which is the rule in this case.

  • files with a single code line under or above 300 chars have the syntax disabled. I can add here the files which are detected as being minified (of .js type) but I also found some html and css:

https://code.addons-dev.allizom.org/en-US/compare/618937/versions/1690424...1690425/?allowSlowPages=false&path=src%2Fbackground%2Findex.html - 149 chars
https://code.addons-dev.allizom.org/en-US/compare/618525/versions/1689954...1689955/?allowSlowPages=false&path=src%2Fbackground%2Findex.html - 167 chars
https://code.addons-dev.allizom.org/en-US/compare/618525/versions/1689954...1689955/?allowSlowPages=false&path=src%2Fconfirm%2Fframe.css - 324 chars
https://code.addons-dev.allizom.org/en-US/compare/618937/versions/1690424...1690425/?allowSlowPages=false&path=src%2Fcontent%2Fengines%2F123rf.js - 252 chars
https://code.addons-dev.allizom.org/en-US/compare/618937/versions/1690424...1690425/?allowSlowPages=false&path=src%2Fcontent%2Fengines%2Fdreamstime.js -
226 chars

This should be fine.

  • files types other than .js which are not detected as being minified but contain a line which has a long block of code have the syntax disabled.

https://code.addons-dev.allizom.org/en-US/compare/618937/versions/1690424...1690425/?allowSlowPages=false&path=src%2Faction%2Fscript.js.map
https://code.addons-dev.allizom.org/en-US/compare/618937/versions/1690424...1690425/?allowSlowPages=false&path=src%2Faction%2Fstyle.css
https://code.addons.allizom.org/en-US/compare/1004516/versions/2477544...2477545/?path=_locales%2Fen%2Fmessages.json

This should also be alright.

I think we don't need to care about minified files because it is a pure code-manager feature. If most common files (fully loaded and that are not crazy long) have the right highlighting, then it's fine.

@willdurand Problem is I don't know if it's the expected. I thought Compare view should have syntax highlighting disabled all the way but perhaps I made a wrong assumption because lately I verified truncation and I've noticed that Compare files had the highlighting disabled.

I'll keep these test results as reference for the future and I'm marking this verified on dev with FF76(Win10).

Perhaps @bobsilverberg could take a look on this and share his opinion. That would be great.

@ioanarusiczki In terms of verifying this issue, we just need to check two things:

  1. That the MIME type in the Information panel is correct.
  2. That if code is highlighted, it seems to be highlighted correctly.

If there are cases where code is not being highlighted, but we expect it to be, then a separate issue should be opened for those cases.

Does that help?

@bobsilverberg I could check the MIME types perhaps I'll find an explanation why some files are highlighted and some are not for the Compare view. I'll get back with results.

@bobsilverberg

Checking MIME Types I don't think anything is wrong.

So, as I said above pages with one single short line have the syntax disabled.
I'll give 2 examples:
https://code.addons-dev.allizom.org/en-US/compare/618937/versions/1690424...1691272/?allowSlowPages=false&path=src%2Fbackground%2Findex.html
https://code.addons-dev.allizom.org/en-US/compare/618937/versions/1690424...1690425/?allowSlowPages=false&path=src%2Fcontent%2Fengines%2Fadobestock.js

Meanwhile there are files which will have syntax disabled at 3000+ lines of code.
I added lines to see what happens - these initially had less than 1000 lines and they were highlighted before.

https://code.addons-dev.allizom.org/en-US/compare/619691/versions/1691091...1691271/?allowSlowPages=true&path=ui%2Fdevtools%2Fstyle.css
https://code.addons-dev.allizom.org/en-US/compare/619691/versions/1691091...1691271/?allowSlowPages=true&path=ui%2Fdevtools%2Findex.js

Maybe Compare view should have highlighting disabled in all situations as Browse pages have it always enabled ?

@ioanarusiczki These are useful findings, but could you please open a separate issue for these, for the code-manager repo. I don't think they have anything to do with this issue.

Was this page helpful?
0 / 5 - 0 ratings