if format is unknown NullMimeTypeObject is returned #8085

Merged
merged 1 commit into from Dec 22, 2012

Conversation

Projects
None yet
6 participants
Contributor

acapilleri commented Oct 31, 2012

If a unknown format is passed in a request, the methods html?, xml?, json? ...etc
Nil Exception.

This patch add a class NullMimeTypeObject, that is returned when request.format is unknown
and it responds false to the methods that ends with '?'.

It refers to #7837, it's considered a improvement not a bug.

Contributor

acapilleri commented Oct 31, 2012

@guilleiguaran I update the PR, it fails here

@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Oct 31, 2012

actionpack/test/dispatch/request_test.rb
@@ -590,7 +590,17 @@ def url_for(options = {})
request = stub_request
request.expects(:parameters).at_least_once.returns({ :format => :unknown })
- assert request.formats.empty?
+ assert_instance_of Mime::NullMimeTypeObject , request.format
+ end
+
+
+ test "format is not nil with unknown format" do
+ request = stub_request
+ request.expects(:parameters).at_least_once.returns({ format: :hello })
+ assert_equal request.format.html?, false
+ assert_equal request.format.xml?, false
+ assert_equal request.format.json?, false
+ assert !request.format.html?
@carlosantoniodasilva

carlosantoniodasilva Oct 31, 2012

Owner

Use assert ! in the other lines, the last one can be removed.

@acapilleri

acapilleri Oct 31, 2012

Contributor

I test that the request.format.html? is equal to false

@carlosantoniodasilva

carlosantoniodasilva Oct 31, 2012

Owner

It's important that it returns a falsy value, that evaluates to false, so !request.format.foo? should suffice I believe.

@carlosantoniodasilva carlosantoniodasilva and 2 others commented on an outdated diff Oct 31, 2012

actionpack/lib/action_dispatch/http/mime_type.rb
@@ -301,6 +301,13 @@ def respond_to_missing?(method, include_private = false) #:nodoc:
method.to_s.ends_with? '?'
end
end
+
+ class NullMimeTypeObject
+ private
+ def method_missing(method, *args)
+ false if method.to_s.ends_with? '?'
@carlosantoniodasilva

carlosantoniodasilva Oct 31, 2012

Owner

Same super comment from previously.

@acapilleri

acapilleri Oct 31, 2012

Contributor

if I add the call to super a many test fails

@carlosantoniodasilva

carlosantoniodasilva Oct 31, 2012

Owner

So that's weird.. Everything green without it?

@acapilleri

acapilleri Oct 31, 2012

Contributor

@guilleiguaran ask to made a new update pr to continue the discussion about this revert. Thanks for your patience

@rafaelfranca

rafaelfranca Oct 31, 2012

Owner

If think it is green because it will return nil instead to raise an NoMethodError and I think we are expecting nil

@acapilleri

acapilleri Oct 31, 2012

Contributor

changed to

 def test_send_file_headers_with_bad_symbol
    options = {
      :type => :this_type_is_not_registered
    }

    @controller.headers = {}
    assert !@controller.send(:send_file_headers!, options)
  end

everything is green

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Oct 31, 2012

actionpack/test/dispatch/request_test.rb
@@ -590,7 +590,17 @@ def url_for(options = {})
request = stub_request
request.expects(:parameters).at_least_once.returns({ :format => :unknown })
- assert request.formats.empty?
+ assert_instance_of Mime::NullMimeTypeObject , request.format
+ end
+
+
@carlosantoniodasilva

carlosantoniodasilva Oct 31, 2012

Owner

✂️ , and remove the extra space before the , in line 593.

Owner

guilleiguaran commented Oct 31, 2012

@carlosantoniodasilva @rails reviewers, this isn't ready for merge yet, I asked for the PR to continue working with @acapilleri in the solution and try to get everything green :)

Contributor

acapilleri commented Nov 7, 2012

@guilleiguaran any news?

Member

steveklabnik commented Dec 15, 2012

This is now out of date, and will need rebased.

@carlosantoniodasilva @guilleiguaran now that everything is green, is this good to go once rebased?

Contributor

acapilleri commented Dec 16, 2012

@steveklabnik in the current PR test_send_file_headers_with_bad_symbol is the only red, but it could be delete because with this PR, `this_type_is_not_registeredshould not raiseArgumentError`` . I'm waiting for a confirm and I will update the PR

Contributor

acapilleri commented Dec 20, 2012

@guilleiguaran any thought?

Owner

guilleiguaran commented Dec 20, 2012

@acapilleri hey, thanks for ping me, I will ping to @jeremy to have some extra eyes in this one before of merge it 😄

Contributor

acapilleri commented Dec 20, 2012

@guilleiguaran 😄, I think that is important my last comment to @steveklabnik, where I explain my doubt,
anyway thanks! Ping me when you will have a feedback and I will update

Owner

pixeltrix commented Dec 20, 2012

@acapilleri I think the NullMimeTypeObject class name is a bit unwieldy - any reason we can't just use NullType since it's namespaced inside of Mime anyway. @guilleiguaran what do you think? Also do we want to return true for nil?.

Contributor

acapilleri commented Dec 20, 2012

@pixeltrix Thanks for you code review, what class name do you think could be fine?

Owner

pixeltrix commented Dec 20, 2012

@acapilleri Mime::NullType should be fine. The reason why I think it should return true for nil? is code like this won't break:

if request.format.nil?
  # do stuff
end

Ideally the NullType object would act like NilClass unfortunately that appears to be hardwired into MRI.

Owner

guilleiguaran commented Dec 21, 2012

@pixeltrix right, I tried it already to fix the issue associated to this PR, isn't possible create a object that acts like NilClass.

@acapilleri Please rebase this and update the name to Mime::NullType and add nil? method. I'll merge this if no one have more concerns about it.

Contributor

acapilleri commented Dec 22, 2012

Member

steveklabnik commented Dec 22, 2012

It still doesn't merge cleanly.

@acapilleri acapilleri return Mime::NullType if format is unknown
If a request has an unknown format, the methods html?, xml?, json? ...etc
not raise an Exception.

This patch add a class Mime::NullType, that is returned when  request.format is unknown
and it responds false to the methods that ends with '?' and true to 'nil?'.

It refers to #7837, this issue is considered a improvement not a bug.
c2267db
Contributor

acapilleri commented Dec 22, 2012

updated thanks.

guilleiguaran merged commit cba0588 into rails:master Dec 22, 2012

Owner

guilleiguaran commented Dec 22, 2012

Thanks for your contribution!!!

@acapilleri acapilleri added a commit to acapilleri/rails that referenced this pull request Nov 29, 2013

@acapilleri acapilleri Fix header Content-Type: #<Mime::NullType:...> in localized template
This PR fixes #13064 regression bug introduced by the #8085

Ideally the NullType object would act like NilClass unfortunately
that appears to be hardwired into MRI, as we know nil.to_a -> []
4ef4771

@acapilleri acapilleri added a commit to acapilleri/rails that referenced this pull request Dec 2, 2013

@acapilleri @acapilleri acapilleri + acapilleri Fix header Content-Type: #<Mime::NullType:...> in localized template
This PR fixes #13064 regression bug introduced by the #8085

Now in _process_format when the format is a Mime::NullType nothing is written in self.content_type.
In this way the method Response#assign_default_content_type_and_charset can
write the the default mime_type.
46a66e4

@acapilleri acapilleri added a commit to acapilleri/rails that referenced this pull request Dec 3, 2013

@acapilleri @acapilleri acapilleri + acapilleri Fix header Content-Type: #<Mime::NullType:...> in localized template
This PR fixes #13064 regression bug introduced by the #8085

Now in _process_format when the format is a Mime::NullType nothing is written in self.content_type.
In this way the method Response#assign_default_content_type_and_charset can
write the the default mime_type.
43962d6

@acapilleri acapilleri added a commit to acapilleri/rails that referenced this pull request Dec 3, 2013

@acapilleri acapilleri Fix header Content-Type: #<Mime::NullType:...> in localized template
This PR fixes #13064 regression bug introduced by the #8085

Now in _process_format when the format is a Mime::NullType nothing is written in self.content_type.
In this way the method Response#assign_default_content_type_and_charset can
write the the default mime_type.
eb448ef

@acapilleri acapilleri added a commit to acapilleri/rails that referenced this pull request Dec 4, 2013

@acapilleri acapilleri Fix header Content-Type: #<Mime::NullType:...> in localized template
This PR is the backport of #13141.
It fixes #13064 regression bug introduced by the #8085.
2c97d32

@carlosantoniodasilva carlosantoniodasilva added a commit to carlosantoniodasilva/rails that referenced this pull request Dec 19, 2013

@carlosantoniodasilva carlosantoniodasilva Move the null mime type to request.format
TLDR: always return an object that responds to the query methods from
request.format, and do not touch Mime::Type[] lookup to avoid bugs.

---

Long version:

The initial issue was about being able to do checks like
request.format.html? for request with an unknown format, where
request.format would be nil.

This is where the issue came from at first in #7837 and #8085
(merged in cba0588), but the
implementation went down the path of adding this to the mime type
lookup logic.

This unfortunately introduced subtle bugs, for instance in the merged
commit a test related to send_file had to be changed to accomodate the
introduction of the NullType.

Later another bug was found in #13064, related to the content-type being
shown as #<Mime::NullType:...> for templates with localized extensions
but no format included. This one was fixed in #13133, merged in
43962d6.

Besides that, custom handlers were not receiving the proper template
formats anymore when passing through the rendering process, because of
the NullType addition. That was found while migrating an application
from 3.2 to 4.0 that uses the Markerb gem (a custom handler that
generates both text and html emails from a markdown template).

---

This changes the implementation moving away from returning this null
object from the mime lookup, and still fixes the initial issue where
request.format.zomg? would raise an exception for unknown formats due to
request.format being nil.
ed60bbd

@carlosantoniodasilva carlosantoniodasilva added a commit to carlosantoniodasilva/rails that referenced this pull request Dec 20, 2013

@carlosantoniodasilva carlosantoniodasilva Move the null mime type to request.format
TLDR: always return an object that responds to the query methods from
request.format, and do not touch Mime::Type[] lookup to avoid bugs.

---

Long version:

The initial issue was about being able to do checks like
request.format.html? for request with an unknown format, where
request.format would be nil.

This is where the issue came from at first in #7837 and #8085
(merged in cba0588), but the
implementation went down the path of adding this to the mime type
lookup logic.

This unfortunately introduced subtle bugs, for instance in the merged
commit a test related to send_file had to be changed to accomodate the
introduction of the NullType.

Later another bug was found in #13064, related to the content-type being
shown as #<Mime::NullType:...> for templates with localized extensions
but no format included. This one was fixed in #13133, merged in
43962d6.

Besides that, custom handlers were not receiving the proper template
formats anymore when passing through the rendering process, because of
the NullType addition. That was found while migrating an application
from 3.2 to 4.0 that uses the Markerb gem (a custom handler that
generates both text and html emails from a markdown template).

---

This changes the implementation moving away from returning this null
object from the mime lookup, and still fixes the initial issue where
request.format.zomg? would raise an exception for unknown formats due to
request.format being nil.

Conflicts:
	actionpack/CHANGELOG.md
	actionpack/lib/abstract_controller/rendering.rb
	actionpack/lib/action_controller/metal/rendering.rb
8ee9414

@carlosantoniodasilva carlosantoniodasilva added a commit that referenced this pull request Dec 23, 2013

@carlosantoniodasilva carlosantoniodasilva Move the null mime type to request.format
TLDR: always return an object that responds to the query methods from
request.format, and do not touch Mime::Type[] lookup to avoid bugs.

---

Long version:

The initial issue was about being able to do checks like
request.format.html? for request with an unknown format, where
request.format would be nil.

This is where the issue came from at first in #7837 and #8085
(merged in cba0588), but the
implementation went down the path of adding this to the mime type
lookup logic.

This unfortunately introduced subtle bugs, for instance in the merged
commit a test related to send_file had to be changed to accomodate the
introduction of the NullType.

Later another bug was found in #13064, related to the content-type being
shown as #<Mime::NullType:...> for templates with localized extensions
but no format included. This one was fixed in #13133, merged in
43962d6.

Besides that, custom handlers were not receiving the proper template
formats anymore when passing through the rendering process, because of
the NullType addition. That was found while migrating an application
from 3.2 to 4.0 that uses the Markerb gem (a custom handler that
generates both text and html emails from a markdown template).

---

This changes the implementation moving away from returning this null
object from the mime lookup, and still fixes the initial issue where
request.format.zomg? would raise an exception for unknown formats due to
request.format being nil.
618d531

@carlosantoniodasilva carlosantoniodasilva added a commit that referenced this pull request Dec 23, 2013

@carlosantoniodasilva carlosantoniodasilva Move the null mime type to request.format
TLDR: always return an object that responds to the query methods from
request.format, and do not touch Mime::Type[] lookup to avoid bugs.

---

Long version:

The initial issue was about being able to do checks like
request.format.html? for request with an unknown format, where
request.format would be nil.

This is where the issue came from at first in #7837 and #8085
(merged in cba0588), but the
implementation went down the path of adding this to the mime type
lookup logic.

This unfortunately introduced subtle bugs, for instance in the merged
commit a test related to send_file had to be changed to accomodate the
introduction of the NullType.

Later another bug was found in #13064, related to the content-type being
shown as #<Mime::NullType:...> for templates with localized extensions
but no format included. This one was fixed in #13133, merged in
43962d6.

Besides that, custom handlers were not receiving the proper template
formats anymore when passing through the rendering process, because of
the NullType addition. That was found while migrating an application
from 3.2 to 4.0 that uses the Markerb gem (a custom handler that
generates both text and html emails from a markdown template).

---

This changes the implementation moving away from returning this null
object from the mime lookup, and still fixes the initial issue where
request.format.zomg? would raise an exception for unknown formats due to
request.format being nil.

Conflicts:
	actionpack/CHANGELOG.md
	actionpack/lib/abstract_controller/rendering.rb
	actionpack/lib/action_controller/metal/rendering.rb
56962ba

@tienn tienn pushed a commit to square/rails that referenced this pull request Jul 2, 2014

@carlosantoniodasilva carlosantoniodasilva + Nelson Crespo Move the null mime type to request.format
TLDR: always return an object that responds to the query methods from
request.format, and do not touch Mime::Type[] lookup to avoid bugs.

---

Long version:

The initial issue was about being able to do checks like
request.format.html? for request with an unknown format, where
request.format would be nil.

This is where the issue came from at first in #7837 and #8085
(merged in cba0588), but the
implementation went down the path of adding this to the mime type
lookup logic.

This unfortunately introduced subtle bugs, for instance in the merged
commit a test related to send_file had to be changed to accomodate the
introduction of the NullType.

Later another bug was found in #13064, related to the content-type being
shown as #<Mime::NullType:...> for templates with localized extensions
but no format included. This one was fixed in #13133, merged in
43962d6.

Besides that, custom handlers were not receiving the proper template
formats anymore when passing through the rendering process, because of
the NullType addition. That was found while migrating an application
from 3.2 to 4.0 that uses the Markerb gem (a custom handler that
generates both text and html emails from a markdown template).

---

This changes the implementation moving away from returning this null
object from the mime lookup, and still fixes the initial issue where
request.format.zomg? would raise an exception for unknown formats due to
request.format being nil.

Conflicts:
	actionpack/CHANGELOG.md
	actionpack/lib/abstract_controller/rendering.rb
	actionpack/lib/action_controller/metal/rendering.rb
cfde5b0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment