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
🇺🇦 Rails 6.1 upgrade #1533
base: main
Are you sure you want to change the base?
🇺🇦 Rails 6.1 upgrade #1533
Conversation
674b8df
to
aaaf6f1
Compare
@stevecrozz Aloha! This PR is ready for first code review. I don't test it in production. Not everything that marked a deprecated are fixed. But, anyway! I have some time for this. :) |
@biow0lf what is left here? I can help you a bit with this. |
For now, I think, I need to build docker image and put it in production to check. Everything that left is rubocop stuff. I don't want to start rubocop war. Updated rubocop just fail to start. But, if I will try update it, I will need to update rubocop configuration and I don't want. Best option: replace rubocop with standardrb gem. |
@nijikon BTW, I am in Poland and looking for job. :) |
abb9c7f
to
bfbdf2c
Compare
I'm happy to take on the rubocop stuff if needed. I'm partial to a few of our rules, although most of standardrb is good with me. |
Let's skip rubocop changes. We can do them in a separate PR. I can also test the new image in production. Please let me know when it is available. |
Docker image with current branch. Ready for testing.
|
@stevecrozz Ok. I think, I made everything for now. Rubocop disabled for now. Circle CI disabled. Time to review and check final build. |
@stevecrozz Ok, 2 weeks later I will say that it works. Can you review and merge? It's already time for more fixes. And first is security updates like |
I haven't forgotten, but I'm not finding the time to give this the attention it deserves. @burnettk do you have the capacity to review this? |
@@ -6,6 +6,7 @@ | |||
class Problem | |||
include Mongoid::Document | |||
include Mongoid::Timestamps | |||
include Mongoid::Attributes::Dynamic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm always leery of this because we lose protection against bugs that may write random fields. What attribute did we need this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@biow0lf any comment on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevecrozz @nijikon if comment this include, test suite fails with:
1) Problem notice hosts cache removing a notice removes string from #hosts
Failure/Error: doc.unset(field)
Mongoid::Errors::UnknownAttribute:
message:
Attempted to set a value for 'messages.460873b7409daabaaa2ed7bece77e52e' which is not allowed on the model Problem.
summary:
Problem#messages.460873b7409daabaaa2ed7bece77e52e= was called but there is no 'messages.460873b7409daabaaa2ed7bece77e52e' field defined in the model, and Mongoid::Attributes::Dynamic is not included. This error is also raised instead of NoMethodError if the unknown attribute is passed to any method that accepts an attributes hash, such as #attributes=.
resolution:
Define the field 'messages.460873b7409daabaaa2ed7bece77e52e' in Problem, or include Mongoid::Attributes::Dynamic in Problem if you intend to store values in fields that are not explicitly defined.
# ./app/models/problem.rb:159:in `block (2 levels) in uncache_notice'
# ./app/models/problem.rb:152:in `each'
# ./app/models/problem.rb:152:in `block in uncache_notice'
# ./app/models/problem.rb:142:in `uncache_notice'
# ./app/models/notice.rb:132:in `problem_recache'
# ./spec/models/problem_spec.rb:382:in `block (4 levels) in <top (required)>'
# ./spec/models/problem_spec.rb:381:in `block (3 levels) in <top (required)>'
I am NOT mongodb guru. I never use mongo db. I just don't understand how to fix this.
Including Mongoid::Attributes::Dynamic
make test suite pass.
And, looks like this is OK. https://jira.mongodb.org/browse/MONGOID-3875
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will have a look at this next week. I don't want all your hard work to go to waste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some digging into this and I think I found the issue and a possible solution.
Problem#uncache_notice
def uncache_notice(notice)
last_notice = notices.last
atomically do |doc|
doc.set(
'environment' => last_notice.environment_name,
'error_class' => last_notice.error_class,
'last_notice_at' => last_notice.created_at,
'message' => last_notice.message,
'where' => last_notice.where,
'notices_count' => notices_count.to_i > 1 ? notices_count - 1 : 0
)
CACHED_NOTICE_ATTRIBUTES.each do |k, v|
digest = Digest::MD5.hexdigest(notice.send(v))
field = "#{k}.#{digest}"
if (doc[k].try(:[], digest).try(:[], :count)).to_i > 1
doc.inc("#{field}.count" => -1)
else
doc.unset(field)
end
end
end
end
doc.unset(field)
in mongoid >= 7.x interprets this as setting an attribute and since there is no attribute called "#{key}.#{digest}" it fails. Previous version of mongoid appear to just update the hash that #{key} pointed to. So, looks like we just need to remove the cached digest from the hash using a different method. This would do it:
if (doc[k].try(:[], digest).try(:[], :count)).to_i > 1
doc.inc("#{field}.count" => -1)
else
h = doc[k] || {}
h.delete(digest)
doc.set("#{k}": h)
end
With this update and removing include Mongoid::Attributes::Dynamic
from Problem
class the entire test suite passes. #1546
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevecrozz, @nijikon with removal of Dynamic attributes, is there anything else that is preventing this PR from getting merged?
I have one single comment regarding Dynamic attributes. I'd love to see if we can get rid of that, or track down why it was needed, and I also put together a PR to your branch to bring rubocop back. We can discuss it there if you have any thoughts on it. |
Just rebuild and push with current changes from this branch. |
Sorry for the delay in replying @biow0lf. I will get back to it during the weekend. I will update my production setup to this branch, and we will go from there. I would love to merge this by the end of October. |
List of changes:
config/initializers/new_framework_defaults.rb
. New rails versions removes this file. Instead, we are doingconfig.load_defaults 6.1
inconfig/application.rb
:classic
.hoptoad_notifier
gem vendored and fixed.