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

Nondeterministic order of MIME::Types.type_for #148

Open
slonopotamus opened this issue Mar 12, 2020 · 9 comments
Open

Nondeterministic order of MIME::Types.type_for #148

slonopotamus opened this issue Mar 12, 2020 · 9 comments

Comments

@slonopotamus
Copy link

slonopotamus commented Mar 12, 2020

I ran a simple test case on multiple Ruby implementations:

    it 'is returns stable order for types with equal priority' do
      assert_equal %w(audio/webm video/webm),
                   MIME::Types.type_for('foo.webm')
    end

On Travis CI, it passes with whole range of Rubies that you test against.

But it fails on my Windows 10 machine because actual return value is reversed (first video/webm, second audio/webm):

  1) Failure:
MIME::Types::registry::.type_for#test_0006_is returns stable order for types with equal priority [C:/Users/Marat/Documents/ruby-mime-types/test/test
_mime_types_class.rb:105]:
Expected: ["audio/webm", "video/webm"]
  Actual: [#<MIME::Type: video/webm>, #<MIME::Type: audio/webm>]

My Ruby is:

$ ruby --version
ruby 2.6.5p114 (2019-10-01 revision 67812) [x64-mingw32]

I also ran this test on GitHub Actions and it fails on Windows too. But note that Ruby--2.2 and 2.3 did pass on Windows.

UPD: I messed up everything a bit so had to update PR since it was initially created.

@halostatue
Copy link
Member

Thanks for the report. I don’t have access to a Windows machine to do this testing on. The implementation of MIME::Types#type_for is currently this:

  def type_for(filename)
    Array(filename).flat_map { |fn|
      @extension_index[fn.chomp.downcase[/\.?([^.]*?)$/, 1]]
    }.compact.inject(Set.new, :+).sort { |a, b|
      a.priority_compare(b)
    }
  end

Can you try this implementation instead?

  def type_for(filename)
    Array(filename).flat_map { |fn|
      @extension_index[fn.chomp.downcase[/\.?([^.]*?)$/, 1]]
    }.compact.uniq.sort { |a, b|
      a.priority_compare(b)
    }
  end

I’m not necessarily expecting any difference

@slonopotamus
Copy link
Author

I had to tweak your suggested variant a bit:

  def type_for(filename)
    Array(filename).flat_map { |fn|
      @extension_index[fn.chomp.downcase[/\.?([^.]*?)$/, 1]].to_a
    }.compact.uniq.sort { |a, b|
      a.priority_compare(b)
    }
  end

As you can see here, the picture is the same - Rubies 2.4+ on Windows get different order.

@slonopotamus
Copy link
Author

slonopotamus commented Mar 15, 2020

I believe the problem is in sort. Its docs say:

The result is not guaranteed to be stable. When the comparison of two elements returns +0+, the order of the elements is unpredictable.

There are ways to make it stable: https://stackoverflow.com/a/15442966

@slonopotamus
Copy link
Author

I found an even more interesting information about sort stability: https://stackoverflow.com/questions/15442298/is-sort-in-ruby-stable/44486562#44486562

@halostatue
Copy link
Member

Good find. I would happily accept a PR that adds stability to the sort, but I have no time to do this myself.

@slonopotamus
Copy link
Author

Okay, I'll work on a PR. Just wanted to be sure that it is accepted as a valid bug.

@halostatue
Copy link
Member

It’s supposed to be stable.

@contentfree
Copy link

Just found this ticket due to the differing order (for "file.mp4" in my case) of returned types. I see there's a PR but a comment on it saying something along the lines of "this might not be performant so I won't merge it".

Any chance of adding a flag / option to types_for / of to get this stable sort behavior in a way that's also backwards compatible?

@halostatue
Copy link
Member

I’ll need to look through the PR again.

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

No branches or pull requests

3 participants