Re: [Rails-core] Cookie store and object instances in session behavior when instance class no longer exists
On 16-07-2015 09:34, Matt Jones wrote:
On Jul 15, 2015, at 11:06 AM, Rodrigo Rosenfeld Rosas rr.ro...@gmail.com
wrote:
Today a colleague was playing in another branch trying the ruby-saml gem to
play with SAML. When he was back to the master branch all requests failed for
apparently no reason. This is related to this (it was trying to instantiate
some OneLogin class which doesn't exist in master):
http://devblog.songkick.com/2012/10/24/get-your-objects-out-of-my-session/
I just think that it's too bad on Rails part to simply crash when it's unable
to deserialize some value from the cookie. I noticed Rack would simply ignore
them and return nil in such cases:
https://github.com/rack/rack/blob/1.6.4/lib/rack/session/cookie.rb#L66
Why not doing the same in Rails?
https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/cookies.rb#L420
I ended up doing something like this in my application:
Rails.application.config.action_dispatch.cookies_serializer = :marshal
module RailsSerializerPatch
protected
def deserialize(name, value)
super
rescue
puts Failed to deserialize session value for #{name}: #{value}
nil
end
end
ActionDispatch::Cookies::EncryptedCookieJar.prepend RailsSerializerPatch
ActionDispatch::Cookies::SignedCookieJar.prepend RailsSerializerPatch
I tried to prepend to SerializedCookieJars module but it didn't work and I have
no idea why, but anyway...
Maybe it would be even better to delete that key from the session whenever such
error happens.
IMO this isn’t a good default behavior. Silently deleting the key - and
muttering to STDOUT is pretty close to “silent” - means that if this issue is
hit in production unexpectedly it is basically invisible.
I agree that even though I'm okay with this behavior for my application,
Rails should provide a better default.
But I don't think the current behavior is a good default either. I think
it's much worse than silently ignore values it can't deserialize.
Currently, the user would no longer be able to use the application or
even to reauthenticate or clean up his cookies if some middleware will
try to load all values, like Devise does... It will simply always raise
500 for the user on all requests until the user manually clear up his
cookies. This is much worse than silently ignoring values that can't be
deserialized.
Shouldn't Rails better handle such deserialization problems without crashing
the whole application on every request? For example, even if I enable Devise
sign-out through get requests the application would crash (respond with 500)
before having the opportunity to clear up any cookies…
In the general case, if the session can’t be cleanly deserialized there’s very
little that can be done.
I wouldn't say that, and I've even provided some suggestions. If
silently ignoring such values is not desired, than maybe Rails should
simply render an error page, so that it gets noticed and then clear up
the current session and force the user to re-authenticate rather than
failing forever. This would be certainly much better than the current
behavior.
A specific application can make the call to muddle through”, but that would
be an unsafe default for some applications.
Or maybe Rails could simply remove all cookies when an error happens due to
deserialization raising. Or at least make the desired behavior more easily
configurable. What do you think?
+1 to making it configurable.
—Matt Jones
I may try to get some time to work on this once I know what would be the
desired options bundled with Rails. Maybe something like:
Rails.application.config.action_dispatch.on_cookies_serializer_load_error =
option # please suggest a better option name
Where option could be :error_once_and_clear_cookies (maybe the default),
:silently_ignore, :raise (current behavior) or maybe a hash like:
{clear_cookies_and_redirect_to: '/'}
Sounds good to you?
Best,
Rodrigo.
--
You received this message because you are subscribed to the Google Groups Ruby on
Rails: Core group.
To unsubscribe from this group and stop receiving emails from it, send an email
to rubyonrails-core+unsubscr...@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/d/optout.
Re: [Rails-core] RFC: Remove support for passing argument to force association reload
OK, so what about the has_one case? Say Project has_one :manager, and project.manager is already loaded (which did a SELECT * FROM managers WHERE project_id = ?), will project.manager.reload do another SELECT * FROM managers WHERE project_id = ?, or will it do a SELECT * FROM managers WHERE id = ?”. To match the has_many I would expect the former, and sometimes we need this behavior if this is the only way to force the association to be looked up again (whereas currently we can do project.manager(true)). But because the object returned by project.manager looks like it is just a plain record instance, I’d expect #reload to behave like the query, a regular find(id). Especially if I call reload(:lock = true). It seems a big ambiguous what the behavior would be for has_one, which makes me think the original project.manager(:reload = true) is clearer - and then reload on the target of a belongs_to or has_one should always just reload that instance, not load a potentially different record. Do you agree? On 16/07/2015, at 23:59 , DHH da...@loudthinking.com wrote: project.documents.reload.first would reload the documents association, then grab the first entry in that array – not trigger another find(id). Like calling #load, but ignoring whether it had already been loaded. On Thursday, July 16, 2015 at 12:53:31 AM UTC+2, will.bryant wrote: So are you saying project.documents.reload.first would redo the query to find the associated record, or would do a find(id) to reload the current instance? On 16/07/2015, at 01:08 , DHH da...@loudthinking.com javascript: wrote: I'd prefer to see us move to a single API, and IMO the trade-offs for #reload alone fits the bill. There's no contract saying that a singular object from has_one/belongs_to and a collection like has_many has to behave the same. In fact, I think it'd be strange to think that it should. A single string and an array of strings do not behave the same. So project.documents.reload.first makes perfect sense to me as reloading the documents collection, then grabbing the first one. projects.owner.reload.name http://projects.owner.reload.name/ makes sense to me as reloading the owner record itself, then fetching the name (if we don't already return self from Record#reload, we should). -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-co...@googlegroups.com javascript:. To post to this group, send email to rubyonra...@googlegroups.com javascript:. Visit this group at http://groups.google.com/group/rubyonrails-core http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com mailto:rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com mailto:rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout.
Re: [Rails-core] RFC: Remove support for passing argument to force association reload
project.documents.reload.first would reload the documents association, then grab the first entry in that array – not trigger another find(id). Like calling #load, but ignoring whether it had already been loaded. On Thursday, July 16, 2015 at 12:53:31 AM UTC+2, will.bryant wrote: So are you saying project.documents.reload.first would redo the query to find the associated record, or would do a find(id) to reload the current instance? On 16/07/2015, at 01:08 , DHH da...@loudthinking.com javascript: wrote: I'd prefer to see us move to a single API, and IMO the trade-offs for #reload alone fits the bill. There's no contract saying that a singular object from has_one/belongs_to and a collection like has_many has to behave the same. In fact, I think it'd be strange to think that it should. A single string and an array of strings do not behave the same. So project.documents.reload.first makes perfect sense to me as reloading the documents collection, then grabbing the first one. projects.owner.reload.name makes sense to me as reloading the owner record itself, then fetching the name (if we don't already return self from Record#reload, we should). -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-co...@googlegroups.com javascript:. To post to this group, send email to rubyonra...@googlegroups.com javascript:. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout.
Re: [Rails-core] RFC: Remove support for passing argument to force association reload
I'd be happy to consider some real code from a real project that shows the use of reloading a single has_one association in a performance hotspot where two ID lookups are proving to be a problem. But for the time being, I'm content with the trade off that has collection#reload and record#reload as the two only ways to reloading those associations. And if someone needs #reload_manager in their model because they do use that a lot, it's trivial to implement on its own. Do appreciate the debate over this, though! Good to go through the reasoning of why we do and make certain changes. On Thursday, July 16, 2015 at 2:28:14 PM UTC+2, will.bryant wrote: I think you’re right that we can’t have complete parity, but then as you said, it would be good to have a single API. It does works well right now having the same API to always get the fresh association loaded, and it is useful to be able to definitely get the current record, even if you don’t personally use it that often :). There is a practical difference between the current project.manager(true) which always does exactly one query, whether or not manager is already loaded, and project.reload.manager, which always does two queries and also changes other stuff on the project instance. Surely it is not necessary to take away the ability to do exactly one association reload? Otherwise you could just as equally argue that there is no use for reload on a has_many. Personally I think I reload has_ones as often as I reload has_manys, and for the same reasons - I need to be sure I am dealing with the current child/children of an object I am halfway through doing stuff to (which is why I don’t want to reload the parent). On 17/07/2015, at 00:17 , DHH da...@loudthinking.com javascript: wrote: I think the first assumption to challenge is that we can have parity between a collection and a single object. I don't think we can or should. An array of strings does not have parity with a single string. So project.manager.reload will call ActiveRecord::Base#reload, so that's a Manager.find(id) call, not a Manager.find_by(project: x) call. If you absolutely need the latter, you can do project.reload.manager. I've found this to be an exceedingly rare case, though. So I'm find have it be a round-about to get there, and I'm fine not having parity between collection and record. On Thursday, July 16, 2015 at 2:09:22 PM UTC+2, will.bryant wrote: OK, so what about the has_one case? Say Project has_one :manager, and project.manager is already loaded (which did a SELECT * FROM managers WHERE project_id = ?), will project.manager.reload do another SELECT * FROM managers WHERE project_id = ?, or will it do a SELECT * FROM managers WHERE id = ?”. To match the has_many I would expect the former, and sometimes we need this behavior if this is the only way to force the association to be looked up again (whereas currently we can do project.manager(true)). But because the object returned by project.manager looks like it is just a plain record instance, I’d expect #reload to behave like the query, a regular find(id). Especially if I call reload(:lock = true). It seems a big ambiguous what the behavior would be for has_one, which makes me think the original project.manager(:reload = true) is clearer - and then reload on the target of a belongs_to or has_one should always just reload that instance, not load a potentially different record. Do you agree? On 16/07/2015, at 23:59 , DHH da...@loudthinking.com wrote: project.documents.reload.first would reload the documents association, then grab the first entry in that array – not trigger another find(id). Like calling #load, but ignoring whether it had already been loaded. On Thursday, July 16, 2015 at 12:53:31 AM UTC+2, will.bryant wrote: So are you saying project.documents.reload.first would redo the query to find the associated record, or would do a find(id) to reload the current instance? On 16/07/2015, at 01:08 , DHH da...@loudthinking.com wrote: I'd prefer to see us move to a single API, and IMO the trade-offs for #reload alone fits the bill. There's no contract saying that a singular object from has_one/belongs_to and a collection like has_many has to behave the same. In fact, I think it'd be strange to think that it should. A single string and an array of strings do not behave the same. So project.documents.reload.first makes perfect sense to me as reloading the documents collection, then grabbing the first one. projects.owner.reload.name makes sense to me as reloading the owner record itself, then fetching the name (if we don't already return self from Record#reload, we should). -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-co...@googlegroups.com.
Re: [Rails-core] RFC: Remove support for passing argument to force association reload
Oh, sorry, I explained that badly. I’m not saying it’s a performance problem doing two queries. I’m saying it’s causing unintended side-effects, because when you do the query on the parent it resets the attributes, including blowing away unsaved changes. This is absolutely undesirable if you just want to make sure you have the current child. How is it trivial to implement reload_manager on its own, BTW? You would have to write out the foreign key SQL, do the query, and then assign back to the association? On 17/07/2015, at 01:07 , DHH da...@loudthinking.com wrote: I'd be happy to consider some real code from a real project that shows the use of reloading a single has_one association in a performance hotspot where two ID lookups are proving to be a problem. But for the time being, I'm content with the trade off that has collection#reload and record#reload as the two only ways to reloading those associations. And if someone needs #reload_manager in their model because they do use that a lot, it's trivial to implement on its own. Do appreciate the debate over this, though! Good to go through the reasoning of why we do and make certain changes. On Thursday, July 16, 2015 at 2:28:14 PM UTC+2, will.bryant wrote: I think you’re right that we can’t have complete parity, but then as you said, it would be good to have a single API. It does works well right now having the same API to always get the fresh association loaded, and it is useful to be able to definitely get the current record, even if you don’t personally use it that often :). There is a practical difference between the current project.manager(true) which always does exactly one query, whether or not manager is already loaded, and project.reload.manager, which always does two queries and also changes other stuff on the project instance. Surely it is not necessary to take away the ability to do exactly one association reload? Otherwise you could just as equally argue that there is no use for reload on a has_many. Personally I think I reload has_ones as often as I reload has_manys, and for the same reasons - I need to be sure I am dealing with the current child/children of an object I am halfway through doing stuff to (which is why I don’t want to reload the parent). On 17/07/2015, at 00:17 , DHH da...@ loudthinking.com http://loudthinking.com/ wrote: I think the first assumption to challenge is that we can have parity between a collection and a single object. I don't think we can or should. An array of strings does not have parity with a single string. So project.manager.reload will call ActiveRecord::Base#reload, so that's a Manager.find(id) call, not a Manager.find_by(project: x) call. If you absolutely need the latter, you can do project.reload.manager. I've found this to be an exceedingly rare case, though. So I'm find have it be a round-about to get there, and I'm fine not having parity between collection and record. On Thursday, July 16, 2015 at 2:09:22 PM UTC+2, will.bryant wrote: OK, so what about the has_one case? Say Project has_one :manager, and project.manager is already loaded (which did a SELECT * FROM managers WHERE project_id = ?), will project.manager.reload do another SELECT * FROM managers WHERE project_id = ?, or will it do a SELECT * FROM managers WHERE id = ?”. To match the has_many I would expect the former, and sometimes we need this behavior if this is the only way to force the association to be looked up again (whereas currently we can do project.manager(true)). But because the object returned by project.manager looks like it is just a plain record instance, I’d expect #reload to behave like the query, a regular find(id). Especially if I call reload(:lock = true). It seems a big ambiguous what the behavior would be for has_one, which makes me think the original project.manager(:reload = true) is clearer - and then reload on the target of a belongs_to or has_one should always just reload that instance, not load a potentially different record. Do you agree? On 16/07/2015, at 23:59 , DHH da...@ loudthinking.com http://loudthinking.com/ wrote: project.documents.reload.first would reload the documents association, then grab the first entry in that array – not trigger another find(id). Like calling #load, but ignoring whether it had already been loaded. On Thursday, July 16, 2015 at 12:53:31 AM UTC+2, will.bryant wrote: So are you saying project.documents.reload.first would redo the query to find the associated record, or would do a find(id) to reload the current instance? On 16/07/2015, at 01:08 , DHH da...@loudthinking.com wrote: I'd prefer to see us move to a single API, and IMO the trade-offs for #reload alone fits the bill. There's no contract saying that a singular object from has_one/belongs_to and a collection like has_many has to behave the same. In
Re: [Rails-core] RFC: Remove support for passing argument to force association reload
I think the first assumption to challenge is that we can have parity between a collection and a single object. I don't think we can or should. An array of strings does not have parity with a single string. So project.manager.reload will call ActiveRecord::Base#reload, so that's a Manager.find(id) call, not a Manager.find_by(project: x) call. If you absolutely need the latter, you can do project.reload.manager. I've found this to be an exceedingly rare case, though. So I'm find have it be a round-about to get there, and I'm fine not having parity between collection and record. On Thursday, July 16, 2015 at 2:09:22 PM UTC+2, will.bryant wrote: OK, so what about the has_one case? Say Project has_one :manager, and project.manager is already loaded (which did a SELECT * FROM managers WHERE project_id = ?), will project.manager.reload do another SELECT * FROM managers WHERE project_id = ?, or will it do a SELECT * FROM managers WHERE id = ?”. To match the has_many I would expect the former, and sometimes we need this behavior if this is the only way to force the association to be looked up again (whereas currently we can do project.manager(true)). But because the object returned by project.manager looks like it is just a plain record instance, I’d expect #reload to behave like the query, a regular find(id). Especially if I call reload(:lock = true). It seems a big ambiguous what the behavior would be for has_one, which makes me think the original project.manager(:reload = true) is clearer - and then reload on the target of a belongs_to or has_one should always just reload that instance, not load a potentially different record. Do you agree? On 16/07/2015, at 23:59 , DHH da...@loudthinking.com javascript: wrote: project.documents.reload.first would reload the documents association, then grab the first entry in that array – not trigger another find(id). Like calling #load, but ignoring whether it had already been loaded. On Thursday, July 16, 2015 at 12:53:31 AM UTC+2, will.bryant wrote: So are you saying project.documents.reload.first would redo the query to find the associated record, or would do a find(id) to reload the current instance? On 16/07/2015, at 01:08 , DHH da...@loudthinking.com wrote: I'd prefer to see us move to a single API, and IMO the trade-offs for #reload alone fits the bill. There's no contract saying that a singular object from has_one/belongs_to and a collection like has_many has to behave the same. In fact, I think it'd be strange to think that it should. A single string and an array of strings do not behave the same. So project.documents.reload.first makes perfect sense to me as reloading the documents collection, then grabbing the first one. projects.owner.reload.name makes sense to me as reloading the owner record itself, then fetching the name (if we don't already return self from Record#reload, we should). -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-co...@googlegroups.com. To post to this group, send email to rubyonra...@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-co...@googlegroups.com javascript:. To post to this group, send email to rubyonra...@googlegroups.com javascript:. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout.
Re: [Rails-core] RFC: Remove support for passing argument to force association reload
I'm comfortable with those trade-offs. Barring any compelling real world code shining a different light on the discussion than what we've covered so far, I don't think we're going to make any additional progress discussing it. On Thursday, July 16, 2015 at 3:12:56 PM UTC+2, will.bryant wrote: Oh, sorry, I explained that badly. I’m not saying it’s a performance problem doing two queries. I’m saying it’s causing unintended side-effects, because when you do the query on the parent it resets the attributes, including blowing away unsaved changes. This is absolutely undesirable if you just want to make sure you have the current child. How is it trivial to implement reload_manager on its own, BTW? You would have to write out the foreign key SQL, do the query, and then assign back to the association? On 17/07/2015, at 01:07 , DHH da...@loudthinking.com javascript: wrote: I'd be happy to consider some real code from a real project that shows the use of reloading a single has_one association in a performance hotspot where two ID lookups are proving to be a problem. But for the time being, I'm content with the trade off that has collection#reload and record#reload as the two only ways to reloading those associations. And if someone needs #reload_manager in their model because they do use that a lot, it's trivial to implement on its own. Do appreciate the debate over this, though! Good to go through the reasoning of why we do and make certain changes. On Thursday, July 16, 2015 at 2:28:14 PM UTC+2, will.bryant wrote: I think you’re right that we can’t have complete parity, but then as you said, it would be good to have a single API. It does works well right now having the same API to always get the fresh association loaded, and it is useful to be able to definitely get the current record, even if you don’t personally use it that often :). There is a practical difference between the current project.manager(true) which always does exactly one query, whether or not manager is already loaded, and project.reload.manager, which always does two queries and also changes other stuff on the project instance. Surely it is not necessary to take away the ability to do exactly one association reload? Otherwise you could just as equally argue that there is no use for reload on a has_many. Personally I think I reload has_ones as often as I reload has_manys, and for the same reasons - I need to be sure I am dealing with the current child/children of an object I am halfway through doing stuff to (which is why I don’t want to reload the parent). On 17/07/2015, at 00:17 , DHH da...@loudthinking.com wrote: I think the first assumption to challenge is that we can have parity between a collection and a single object. I don't think we can or should. An array of strings does not have parity with a single string. So project.manager.reload will call ActiveRecord::Base#reload, so that's a Manager.find(id) call, not a Manager.find_by(project: x) call. If you absolutely need the latter, you can do project.reload.manager. I've found this to be an exceedingly rare case, though. So I'm find have it be a round-about to get there, and I'm fine not having parity between collection and record. On Thursday, July 16, 2015 at 2:09:22 PM UTC+2, will.bryant wrote: OK, so what about the has_one case? Say Project has_one :manager, and project.manager is already loaded (which did a SELECT * FROM managers WHERE project_id = ?), will project.manager.reload do another SELECT * FROM managers WHERE project_id = ?, or will it do a SELECT * FROM managers WHERE id = ?”. To match the has_many I would expect the former, and sometimes we need this behavior if this is the only way to force the association to be looked up again (whereas currently we can do project.manager(true)). But because the object returned by project.manager looks like it is just a plain record instance, I’d expect #reload to behave like the query, a regular find(id). Especially if I call reload(:lock = true). It seems a big ambiguous what the behavior would be for has_one, which makes me think the original project.manager(:reload = true) is clearer - and then reload on the target of a belongs_to or has_one should always just reload that instance, not load a potentially different record. Do you agree? On 16/07/2015, at 23:59 , DHH da...@loudthinking.com wrote: project.documents.reload.first would reload the documents association, then grab the first entry in that array – not trigger another find(id). Like calling #load, but ignoring whether it had already been loaded. On Thursday, July 16, 2015 at 12:53:31 AM UTC+2, will.bryant wrote: So are you saying project.documents.reload.first would redo the query to find the associated record, or would do a find(id) to reload the current instance? On 16/07/2015, at 01:08 , DHH da...@loudthinking.com
Re: [Rails-core] RFC: Remove support for passing argument to force association reload
I think you’re right that we can’t have complete parity, but then as you said, it would be good to have a single API. It does works well right now having the same API to always get the fresh association loaded, and it is useful to be able to definitely get the current record, even if you don’t personally use it that often :). There is a practical difference between the current project.manager(true) which always does exactly one query, whether or not manager is already loaded, and project.reload.manager, which always does two queries and also changes other stuff on the project instance. Surely it is not necessary to take away the ability to do exactly one association reload? Otherwise you could just as equally argue that there is no use for reload on a has_many. Personally I think I reload has_ones as often as I reload has_manys, and for the same reasons - I need to be sure I am dealing with the current child/children of an object I am halfway through doing stuff to (which is why I don’t want to reload the parent). On 17/07/2015, at 00:17 , DHH da...@loudthinking.com wrote: I think the first assumption to challenge is that we can have parity between a collection and a single object. I don't think we can or should. An array of strings does not have parity with a single string. So project.manager.reload will call ActiveRecord::Base#reload, so that's a Manager.find(id) call, not a Manager.find_by(project: x) call. If you absolutely need the latter, you can do project.reload.manager. I've found this to be an exceedingly rare case, though. So I'm find have it be a round-about to get there, and I'm fine not having parity between collection and record. On Thursday, July 16, 2015 at 2:09:22 PM UTC+2, will.bryant wrote: OK, so what about the has_one case? Say Project has_one :manager, and project.manager is already loaded (which did a SELECT * FROM managers WHERE project_id = ?), will project.manager.reload do another SELECT * FROM managers WHERE project_id = ?, or will it do a SELECT * FROM managers WHERE id = ?”. To match the has_many I would expect the former, and sometimes we need this behavior if this is the only way to force the association to be looked up again (whereas currently we can do project.manager(true)). But because the object returned by project.manager looks like it is just a plain record instance, I’d expect #reload to behave like the query, a regular find(id). Especially if I call reload(:lock = true). It seems a big ambiguous what the behavior would be for has_one, which makes me think the original project.manager(:reload = true) is clearer - and then reload on the target of a belongs_to or has_one should always just reload that instance, not load a potentially different record. Do you agree? On 16/07/2015, at 23:59 , DHH da...@ loudthinking.com http://loudthinking.com/ wrote: project.documents.reload.first would reload the documents association, then grab the first entry in that array – not trigger another find(id). Like calling #load, but ignoring whether it had already been loaded. On Thursday, July 16, 2015 at 12:53:31 AM UTC+2, will.bryant wrote: So are you saying project.documents.reload.first would redo the query to find the associated record, or would do a find(id) to reload the current instance? On 16/07/2015, at 01:08 , DHH da...@loudthinking.com wrote: I'd prefer to see us move to a single API, and IMO the trade-offs for #reload alone fits the bill. There's no contract saying that a singular object from has_one/belongs_to and a collection like has_many has to behave the same. In fact, I think it'd be strange to think that it should. A single string and an array of strings do not behave the same. So project.documents.reload.first makes perfect sense to me as reloading the documents collection, then grabbing the first one. projects.owner.reload.name http://projects.owner.reload.name/ makes sense to me as reloading the owner record itself, then fetching the name (if we don't already return self from Record#reload, we should). -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-co...@ googlegroups.com http://googlegroups.com/. To post to this group, send email to rubyonra...@ googlegroups.com http://googlegroups.com/. Visit this group at http://groups.google.com/group/rubyonrails-core http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups Ruby on Rails: Core group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-co...@googlegroups.com . To post to this group, send email to
Re: [Rails-core] Cookie store and object instances in session behavior when instance class no longer exists
On Jul 15, 2015, at 11:06 AM, Rodrigo Rosenfeld Rosas rr.ro...@gmail.com
wrote:
Today a colleague was playing in another branch trying the ruby-saml gem to
play with SAML. When he was back to the master branch all requests failed for
apparently no reason. This is related to this (it was trying to instantiate
some OneLogin class which doesn't exist in master):
http://devblog.songkick.com/2012/10/24/get-your-objects-out-of-my-session/
I just think that it's too bad on Rails part to simply crash when it's unable
to deserialize some value from the cookie. I noticed Rack would simply ignore
them and return nil in such cases:
https://github.com/rack/rack/blob/1.6.4/lib/rack/session/cookie.rb#L66
Why not doing the same in Rails?
https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/cookies.rb#L420
I ended up doing something like this in my application:
Rails.application.config.action_dispatch.cookies_serializer = :marshal
module RailsSerializerPatch
protected
def deserialize(name, value)
super
rescue
puts Failed to deserialize session value for #{name}: #{value}
nil
end
end
ActionDispatch::Cookies::EncryptedCookieJar.prepend RailsSerializerPatch
ActionDispatch::Cookies::SignedCookieJar.prepend RailsSerializerPatch
I tried to prepend to SerializedCookieJars module but it didn't work and I
have no idea why, but anyway...
Maybe it would be even better to delete that key from the session whenever
such error happens.
IMO this isn’t a good default behavior. Silently deleting the key - and
muttering to STDOUT is pretty close to “silent” - means that if this issue is
hit in production unexpectedly it is basically invisible.
Shouldn't Rails better handle such deserialization problems without crashing
the whole application on every request? For example, even if I enable Devise
sign-out through get requests the application would crash (respond with 500)
before having the opportunity to clear up any cookies…
In the general case, if the session can’t be cleanly deserialized there’s very
little that can be done. A specific application can make the call to muddle
through”, but that would be an unsafe default for some applications.
Or maybe Rails could simply remove all cookies when an error happens due to
deserialization raising. Or at least make the desired behavior more easily
configurable. What do you think?
+1 to making it configurable.
—Matt Jones
--
You received this message because you are subscribed to the Google Groups Ruby
on Rails: Core group.
To unsubscribe from this group and stop receiving emails from it, send an email
to rubyonrails-core+unsubscr...@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/d/optout.
signature.asc
Description: Message signed with OpenPGP using GPGMail
[Rails-core] Add ActiveRecord::Relation#each_slice to divide a relation into multiple smaller ones
Hello everyone!
There have been many times when I needed to work with a relation in small
chunks. E.g. deleting or updating large number of rows while allowing the
database to work on other tasks in between. This is specially useful if the
number of affected rows is very large and we need to throttle our work in
order to allow other process to start/complete in between each iteration.
It could also be used to process each slice using concurrent workers.
For example:
People.where('age 21').each_slice do |relation|
sleep 10 # other tasks
relation.update_all(should_party: true)
end
Called without a block, it will return an Enumerator, e.g.:
Poeple.where('age 18').each_slice.each(:delete_all)
Other people have expressed interested in having such functionality as
well. E.g., with a quick GitHub Issues search I found issues #20820 and
#13147. There have been other attempts to achieve the same goal, however
they were backward-incompatible, not tested and not optimised.
I have written passing tests in a similar style to #find_in_batches. I have
made sure that the number database queries is kept low (it is equal and in
some cases less than that of #find_in_batches).
I have documented the method as well.
I have tried to stick to the coding style and conventions of Rails. Please
let me know if I need to make any modifications, or rename it so that I
could get prepare it for getting merged.
PS. let me know what name you prefer for the method, e.g. #batches,
#to_batches, #in_batches, #split, #each_slice, etc. So far, I have called
it #each_slice because it is dividing a single ActiveRecord::Relation
object into multiple ActiveRecord::Relation objects, similar to the more
familiar Array#each_slice, which divides an Enumerable into multiple
Enumerable objects.
Here is the link to the the pull request:
https://github.com/rails/rails/pull/20899
Thanks in advance for your help!
Regards,
Sina
--
You received this message because you are subscribed to the Google Groups Ruby
on Rails: Core group.
To unsubscribe from this group and stop receiving emails from it, send an email
to rubyonrails-core+unsubscr...@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/d/optout.
