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

AO3-6598 Slow query in challenge_signups#update and challenge_signups#create #4720

Merged
merged 9 commits into from
Feb 17, 2024

Conversation

ceithir
Copy link
Contributor

@ceithir ceithir commented Jan 20, 2024

Pull Request Checklist

Issue

https://otwarchive.atlassian.net/browse/AO3-6598

Purpose

Reduce number of lengthy DB requests when creating/updating a challenge
Went for the lazy minimalistic approach there, with behavior basically untouched: a request that was systemically done is just done earlier, and now triggers an explicit early return

Testing Instructions

Try answering a challenge with a prompt having no tag, only character tags, a mix of different kind of tags, etc.

Credit

Ceithir (he/him)

unless disallowed_taglist.empty?
errors.add(:base, ts("^These %{tag_label} tags in your %{prompt_type} are not allowed in this challenge: %{taglist}",
tag_label: tag_type_label_name(tag_type).downcase,
prompt_type: self.class.name.downcase,
taglist: disallowed_taglist.collect(&:name).join(ArchiveConfig.DELIMITER_FOR_OUTPUT)))
end
else
noncanonical_taglist = tag_set.send("#{tag_type}_taglist").reject {|t| t.canonical}
noncanonical_taglist = taglist.reject {|t| t.canonical}
unless noncanonical_taglist.empty?
errors.add(:base, ts("^These %{tag_label} tags in your %{prompt_type} are not canonical and cannot be used in this challenge: %{taglist}. To fix this, please ask your challenge moderator to set up a tag set for the challenge. New tags can be added to the tag set manually by the moderator or through open nominations.",
tag_label: tag_type_label_name(tag_type).downcase,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From how I understand it, if the challenge has no restriction at all, we don't check for canonicity, but if it has any sort of restriction, we make sure all tags are (at least) canonical? Is that intended behavior?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My knowledge of the challenge code is extremely rusty, but the Tag Options part of the Gift Exchange FAQ might help explain the intended/current behavior. The tl;dr is tags shouldn't need to be canonical if they're in one of the challenge's tag sets.

end

it "shouldn't make needless checks on unused kind of tags" do
expect_any_instance_of(PromptRestriction).to receive(:has_tags?).with("character").at_least(:once)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure where the second call to this method happens actually, might be worth investigating.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, simply because the validations are called a first time through valid? then a second time on save.

Let's try to see if we can run them all only once without breaking everything.

Copy link
Contributor

@Bilka2 Bilka2 Feb 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also looking at this. My in-progress comment:

The validation is called twice in prompts_controller#create, once by @challenge_signup.valid? and once by @prompt.save.

This happens because challenge_signup uses accepts_nested_attributes_for :offers, :prompts, :requests. accepts_nested_attributes_for means the associated record is autosaved and thus validated if the parent is validated: https://api.rubyonrails.org/v6.1.7.4/classes/ActiveRecord/AutosaveAssociation.html.

So, prompts_controller#create can use @prompt.save(validate: false) instead.

Your solution of combining it also looks okay, but I'm a bit hesitant to call save on the signup when it's the prompt that is getting created. Because this seems to be barely covered in tests, I think it may be better go for my more conservative solution.

Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the validation the only functional change is the next if taglist.empty? line, which does all the skipping mentioned on Jira.

Combined with no longer calling the validation twice, I think this is a large performance improvement already and we don't need to look at adding indices for now.

@brianjaustin brianjaustin merged commit 735bb29 into otwcode:master Feb 17, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants