clear-code / redmine_full_text_search

Full text search for Redmine
MIT License
61 stars 24 forks source link

Added registered_at to fts_targets as preparation for sort by creation time #118

Closed otegami closed 8 months ago

otegami commented 8 months ago

Related: https://github.com/clear-code/redmine_full_text_search/issues/115

What I did

What I checked

I checked whether I can roll back this migration.

% bin/rake redmine:plugins:migrate VERSION=0 NAME=full_text_search
% bin/rake redmine:plugins:migrate

Ask for reviewers

I struggled with deciding how to update the registered_at field for fts_targets records related to the changes and changesets tables. If you have a good idea, please let me know!

otegami commented 8 months ago

Could you enable GitHub Actions on your fork?

@kou Thanks. I've just enabled it here: https://github.com/otegami/redmine_full_text_search/actions

otegami commented 8 months ago

I will check it tomorrow morning again!

otegami commented 8 months ago

Thank you for reviewing🙏


@kou I am concerned about using the created_at column because these names suggest automatic updating on Rails context, which might lead to misunderstandings. I'm trying to think of an alternative name. And I'm considering registered_at as it seems close to created_at. What do you think?

otegami commented 8 months ago

@kou Thank you for reviewing🙏


I think that registered_at isn't so bad but do you have more suggestions?

How about the following list? In my opinion, the meaning of registered_at or recorded_at seems close to created_at in this context.

otegami commented 8 months ago

@kou Thank you for reviewing!


I wanted to generate queries without embedding variables in SQL but I couldn't find the way to do that. The following code is what I tried for it.

types = [Issue, Message, Project, WikiPage]
fts_target_table = FtsTarget.arel_table

types.each do |type|
  subquery_for_created_on = type.where(type.arel_table[:id].eq(fts_target_table[:source_id]))
                                .select(:created_on)
  subquery_for_fts_type_id = FtsType.where(name: type.table_name.singularize.camelize)
                                    .select(:id)

  manager = Arel::UpdateManager.new
  manager.table(fts_target_table)
  manager.set([[fts_target_table[:registered_at], Arel::Nodes::Grouping.new(subquery_for_created_on)]])
  manager.where(fts_target_table[:source_type_id].in(Arel::Nodes::Grouping.new(subquery_for_fts_type_id)))

  ActiveRecord::Base.connection.execute(manager.to_sql)
end
kou commented 8 months ago

FYI: Our priorities in order:

  1. We want to use Active Record API instead of direct SQL (embedding a string without escaping) as much as possible for safety (we want to use auto escaping)
  2. If we need to use Arel API instead of Active Record API, we want to use it as little as possible for easy to maintain
  3. If we need to use direct SQL, we must escape all dynamic values explicitly for safety
otegami commented 8 months ago

Thank you for reviewing. I understood the priorities. Thank you so much!

otegami commented 8 months ago

@kou

Could you update the PR title and description based on the latest design?

Thanks, I've just updated it.


Thank you for reviewing. I think this PR is ready for merge if you don't have any other concerns.