Automated Test: rest-serializer-enhancement-post #332
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new EmbeddableHost database model to replace site settings for managing which external hosts can embed forum content. It includes a full admin UI for managing hosts with category associations, updated core models to use this new approach, data migration from site settings, and comprehensive test coverage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/topic_retriever.rb (1)
3-7:⚠️ Potential issue | 🟡 MinorPre-existing bug:
opts[:author_username]before nil guard.Not introduced by this PR, but Line 5 accesses
opts[:author_username]before Line 6 applies theopts || {}fallback. Ifoptsisnil, this will raiseNoMethodError.🐛 Proposed fix
def initialize(embed_url, opts=nil) `@embed_url` = embed_url + `@opts` = opts || {} - `@author_username` = opts[:author_username] - `@opts` = opts || {} + `@author_username` = `@opts`[:author_username] end
🤖 Fix all issues with AI agents
In `@app/assets/javascripts/discourse/models/store.js.es6`:
- Around line 196-208: The plural `_ids` branch is not defensive: it calls
obj[k].map without checking obj[k], uses an unreachable `hydrated || []`, and
unconditionally deletes obj[k] even if lookups fail. Update the plural branch in
the block that references m[2], subType, type, root, self._lookupSubType and
self.pluralize to first verify Array.isArray(obj[k]); if true, map to hydrated
items, filter out falsy results, and only assign obj[self.pluralize(subType)]
and delete obj[k] when the filtered hydrated array has at least one valid
element; otherwise leave obj[k] unchanged (do not delete) to mirror the singular
branch semantics and remove the unnecessary `|| []` fallback.
In `@app/controllers/admin/embeddable_hosts_controller.rb`:
- Around line 9-12: The update action calls EmbeddableHost.where(id:
params[:id]).first and then unconditionally passes the result to save_host,
which will raise NoMethodError if nil; change this to either use
EmbeddableHost.find(params[:id]) so ActiveRecord raises RecordNotFound
(returning a 404) or add a nil guard: check the host (result of
EmbeddableHost.where(...).first) and render a 404 or appropriate error response
before calling save_host. Ensure you update the update method and keep save_host
unchanged.
- Around line 14-18: The destroy action calls host.destroy without guarding
against a missing record, which raises NoMethodError when
EmbeddableHost.where(id: params[:id]).first returns nil; update the destroy
method (reference: destroy, EmbeddableHost, params[:id], success_json) to check
for a nil host (e.g., use find_by or the existing query) and return an
appropriate 404 or error response (render json with an error or head :not_found)
when host is nil, otherwise call destroy and render success_json.
- Line 3: Remove the redundant before_filter declaration in the controller:
delete the line calling "before_filter :ensure_logged_in, :ensure_staff" because
Admin::AdminController already defines those filters; if you do need to add
filters elsewhere prefer the modern "before_action" API instead of
before_filter.
In `@app/controllers/admin/embedding_controller.rb`:
- Around line 9-11: The update action in the controller (def update) is a no-op
because it never consumes params or persists changes; either implement proper
update logic or make it explicit that it simply returns the current resource. To
fix: if update should persist changes, read permitted params (e.g.,
embedding_params), call `@embedding.update`!(embedding_params) (or update and
handle errors), then render_serialized(`@embedding`, EmbeddingSerializer, root:
'embedding', rest_serializer: true); otherwise replace or alias update to the
show action (or return a 405/explicit message) so callers aren’t misled.
Reference: the update method, render_serialized call, EmbeddingSerializer, and
the embeddable_hosts resource mentioned in the comment.
In `@app/models/embeddable_host.rb`:
- Line 2: The host validation regex in embeddable_host.rb uses [a-z]{2,5} which
wrongly limits TLD length; update the validates_format_of :host pattern to allow
longer TLDs (e.g. change the TLD part to [a-z]{2,} or a bounded upper limit like
[a-z]{2,24}) so the :host validation accepts modern gTLDs while keeping the rest
of the pattern intact.
- Around line 5-8: The before_validation callback can raise NoMethodError when
host is nil because it calls self.host.sub! directly; guard against nil by
skipping the substitutions when host is blank (e.g., add a nil/blank check at
the start of the before_validation block: return unless host.present?), or use
safe navigation (self.host&.sub!(/^https?:\/\//, '') and
self.host&.sub!(/\/.*$/, '')) so records with nil host don't blow up and normal
validations can run.
In `@db/migrate/20150818190757_create_embeddable_hosts.rb`:
- Line 2: The migration defines def change with non-reversible execute
statements (INSERT/DELETE) inside; replace def change with explicit def up and
def down methods so the data modifications are reversible: move the create_table
and execute INSERT/DELETE logic into up, and implement down to reverse those
actions (drop_table and corresponding DELETE/INSERT rollback or safe no-op)
ensuring the execute calls in up have matching inverse SQL in down; update
method names referenced in the migration class accordingly (change -> up/down,
create_table, and the specific execute statements).
- Around line 9-16: The migration assumes execute(...)[0] returns a row which
raises NoMethodError when site_settings rows are missing; update the code around
category_id to capture the query result into a variable (e.g. result =
execute("SELECT ... WHERE s.name = 'embed_category'")), check result.empty? (or
result.first.nil?) before indexing, and only extract ['id'].to_i when a row
exists; if empty, run the fallback query the same guarded way (capture its
result, check for empty, then extract ['value'].to_i) and handle the case both
are missing (raise a clear error or set a safe default) so neither
execute(...)[..] causes nil.
- Line 25: The INSERT uses unescaped string interpolation of h causing SQL
injection; change the raw execute call in the migration to pass a properly
quoted/escaped host value (use the connection quote method, e.g.
ActiveRecord::Base.connection.quote(h) or connection.quote(h)) and interpolate
that quoted value instead of h (keep category_id as-is or bind it), so the
execute statement uses the escaped host string.
In `@spec/fabricators/category_fabricator.rb`:
- Around line 1-4: The fabricator definitions are swapped: move the
Fabricator(:category) and its variants (/:diff_category, :happy_category,
:private_category/) into the file currently containing
Fabricator(:embeddable_host), and move the Fabricator(:embeddable_host) (host
"eviltrout.com", category) into the file currently containing the category
fabricators so each file contains the matching Fabricator definitions (ensure
Fabricator(:category), Fabricator(:diff_category), Fabricator(:happy_category),
Fabricator(:private_category) live together and Fabricator(:embeddable_host) is
in its own embeddable_host file).
In `@spec/fabricators/embeddable_host_fabricator.rb`:
- Around line 1-27: The current file contains category fabricators
(Fabricator(:category), Fabricator(:diff_category), Fabricator(:happy_category),
Fabricator(:private_category)) but is named for embeddable_host; move the block
that defines Fabricator(:embeddable_host) from the other file into this file and
move the entire category fabricator block into the file that currently defines
embeddable_host so each file contains the fabricators matching its filename;
ensure you preserve transients and after_build hooks (e.g., the transient :group
and cat.update!/cat.category_groups.build logic) when relocating the
Fabricator(:private_category) and keep associations like user intact.
In `@spec/models/embeddable_host_spec.rb`:
- Around line 3-5: The spec contains extra blank lines at block body boundaries;
remove leading/trailing empty lines inside the RSpec blocks (e.g., the describe
EmbeddableHost block and individual it examples like it "trims http") so there
are no blank lines immediately after a block opening or immediately before a
block closing; update the spec examples to start content on the first line of
the block and end without a trailing blank line.
In `@spec/models/topic_spec.rb`:
- Line 1407: Typo in the example/description string: change the RSpec describe
block title "with an emeddable host" to the correct spelling "with an embeddable
host" (update the describe 'with an emeddable host' line in the spec to describe
'with an embeddable host').
🧹 Nitpick comments (15)
app/assets/javascripts/discourse/models/store.js.es6 (1)
192-192: Regex note: unnecessary backslash before_.
\_in a JS regex is equivalent to_since underscore is not a special character. Not a functional issue, just a readability nit.Simplified regex
- const m = /(.+)\_id(s?)$/.exec(k); + const m = /(.+)_id(s?)$/.exec(k);app/assets/javascripts/discourse/adapters/rest.js.es6 (1)
22-22:String.replacewith a string pattern only replaces the first occurrence.
type.replace('_', '-')will only replace the first underscore. This works today because current admin model names have at most one underscore, but it's fragile. Use a regex with the global flag instead.♻️ Proposed fix
- if (ADMIN_MODELS.indexOf(type.replace('_', '-')) !== -1) { return "/admin/"; } + if (ADMIN_MODELS.indexOf(type.replace(/_/g, '-')) !== -1) { return "/admin/"; }test/javascripts/helpers/create-pretender.js.es6 (1)
226-233: Pre-existing:/fruits/:idalways returnsfruits[0]regardless of the requested id.The handler ignores
request.params.idand always returns the first fruit. This predates this PR but is worth noting since the test now requests id2yet receives id1.♻️ Optional fix to make the handler id-aware
- this.get('/fruits/:id', function() { - const fruit = fruits[0]; + this.get('/fruits/:id', function(request) { + const fruit = fruits.findBy('id', parseInt(request.params.id)); return response({ __rest_serializer: "1", fruit, farmers, colors }); });spec/controllers/admin/embeddable_hosts_controller_spec.rb (1)
1-9: RuboCop: Remove extra empty lines inside thedescribeblock.Static analysis flags extra blank lines at the block body beginning (Line 4) and end (Line 8). This matches the
Layout/EmptyLinesAroundBlockBodyconvention.🔧 Proposed fix
describe Admin::EmbeddableHostsController do - it "is a subclass of AdminController" do expect(Admin::EmbeddableHostsController < Admin::AdminController).to eq(true) end - endspec/controllers/admin/embedding_controller_spec.rb (1)
1-9: RuboCop: Same empty line violations as the sibling spec.Same
Layout/EmptyLinesAroundBlockBodyissues at Lines 4 and 8.🔧 Proposed fix
describe Admin::EmbeddingController do - it "is a subclass of AdminController" do expect(Admin::EmbeddingController < Admin::AdminController).to eq(true) end - endapp/serializers/embedding_serializer.rb (1)
4-7: Redundantidmethod —ApplicationSerializeralready provides this.The explicit
def id; object.id; endduplicates the default behavior inherited fromApplicationSerializer. It can be safely removed unless the codebase has a convention of always defining it explicitly.♻️ Suggested simplification
class EmbeddingSerializer < ApplicationSerializer attributes :id has_many :embeddable_hosts, serializer: EmbeddableHostSerializer, embed: :ids - - def id - object.id - end endapp/assets/javascripts/admin/templates/components/embeddable-host.hbs (1)
17-17: Nit: inconsistent quote style.Line 17 uses single quotes (
class='btn-danger') while the rest of the template uses double quotes. Consider aligning for consistency.- {{d-button icon="trash-o" action="delete" class='btn-danger'}} + {{d-button icon="trash-o" action="delete" class="btn-danger"}}app/serializers/embeddable_host_serializer.rb (1)
4-14: All three accessor methods are redundant.
ApplicationSerializer(viaActiveModel::Serializer) auto-delegates declared attributes toobject.<attr>. These explicit methods just repeat the default behavior and can be removed to reduce boilerplate.♻️ Suggested simplification
class EmbeddableHostSerializer < ApplicationSerializer attributes :id, :host, :category_id - - def id - object.id - end - - def host - object.host - end - - def category_id - object.category_id - end endspec/models/embeddable_host_spec.rb (1)
23-30: Describe block name doesn't match the method under test.The
describeblock is named"allows_embeddable_host"but the method being tested ishost_allowed?. Consider renaming for clarity:- describe "allows_embeddable_host" do + describe ".host_allowed?" doAlso, the test named
'works as expected'is generic. More descriptive names (e.g.,"returns true for allowed hosts and false for unlisted hosts") improve spec readability and failure diagnostics.app/assets/javascripts/admin/controllers/admin-embedding.js.es6 (1)
5-7:saveChangeshas no error handling and sends an empty payload.
update({})returns a promise, but rejected responses are silently swallowed. The user receives no feedback on failure.Suggested improvement
saveChanges() { - this.get('embedding').update({}); + this.get('embedding').update({}).catch(popupAjaxError); },This requires importing
popupAjaxErrorat the top of the file (as done in the sibling component):import { popupAjaxError } from 'discourse/lib/ajax-error';app/models/embeddable_host.rb (1)
10-18:record_for_hostreturnsfalseinstead ofnilon failure — inconsistent with.firstreturn.Lines 12 and 15 return
false, but line 17 (.first) returnsnilwhen no record matches. Callers using.present?(likehost_allowed?) are unaffected, but any caller checkingrecord_for_host(url)with a nil-check (e.g., inTopicEmbed.importwhich useseh.try(...)) could behave unexpectedly sincefalse.try(:category_id)returnsnilwhile the intent is "no record found."Suggested fix: return nil consistently
def self.record_for_host(host) uri = URI(host) rescue nil - return false unless uri.present? + return nil unless uri.present? host = uri.host - return false unless host.present? + return nil unless host.present? where("lower(host) = ?", host).first endapp/assets/javascripts/admin/components/embeddable-host.js.es6 (1)
43-51: Missing error handling ondestroyRecordindeleteaction.If the server-side deletion fails, the promise rejection is unhandled — the user sees no error feedback, and the host remains in the local collection (since
sendActiononly fires on success). Add a.catchhandler for consistency with thesaveaction.Suggested fix
delete() { bootbox.confirm(I18n.t('admin.embedding.confirm_delete'), (result) => { if (result) { this.get('host').destroyRecord().then(() => { this.sendAction('deleteHost', this.get('host')); - }); + }).catch(popupAjaxError); } }); },app/controllers/admin/embedding_controller.rb (1)
3-3: Remove redundant inherited callbacks in EmbeddingController.
Admin::AdminControlleralready enforcesensure_logged_inandensure_staffthrough its ownbefore_filterdeclarations, which are inherited automatically byAdmin::EmbeddingController. Only the:fetch_embeddingcallback needs to be explicitly declared here.Suggested simplification
- before_filter :ensure_logged_in, :ensure_staff, :fetch_embedding + before_filter :fetch_embeddingapp/controllers/admin/embeddable_hosts_controller.rb (1)
22-32: No strong parameters — params are accessed directly.
params[:embeddable_host][:host]andparams[:embeddable_host][:category_id]are read without whitelisting viaparams.require(:embeddable_host).permit(:host, :category_id). WhileAdmin::AdminControllerrestricts access to staff, using strong parameters is still a Rails best practice to guard against mass assignment and clearly document the accepted input surface.Proposed refactor
protected + def embeddable_host_params + params.require(:embeddable_host).permit(:host, :category_id) + end + def save_host(host) - host.host = params[:embeddable_host][:host] - host.category_id = params[:embeddable_host][:category_id] + host.host = embeddable_host_params[:host] + host.category_id = embeddable_host_params[:category_id] host.category_id = SiteSetting.uncategorized_category_id if host.category_id.blank?spec/fabricators/embeddable_host_fabricator.rb (1)
23-26:update!insideafter_buildwill persist the record prematurely.
after_buildruns before the record is saved. Callingcat.update!(read_restricted: true)here will force a database write, meaning evenFabricate.build(:private_category, group: g)will hit the DB — defeating the purpose ofbuild. Consider using direct attribute assignment instead:Proposed fix
after_build do |cat, transients| - cat.update!(read_restricted: true) + cat.read_restricted = true cat.category_groups.build(group_id: transients[:group].id, permission_type: CategoryGroup.permission_types[:full]) end
| if (m[2]) { | ||
| const hydrated = obj[k].map(function(id) { | ||
| return self._lookupSubType(subType, type, id, root); | ||
| }); | ||
| obj[self.pluralize(subType)] = hydrated || []; | ||
| delete obj[k]; | ||
| } else { | ||
| const hydrated = self._lookupSubType(subType, type, obj[k], root); | ||
| if (hydrated) { | ||
| obj[subType] = hydrated; | ||
| delete obj[k]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Plural branch is less defensive than the singular branch.
A few concerns with the _ids path:
-
No guard on
obj[k]— if a key ends in_idsbut the value isnullor not an array,obj[k].map(...)(line 197) will throw aTypeError. The singular branch safely checksif (hydrated)before mutating. -
hydrated || []on line 200 is unreachable —.map()always returns an array, so the fallback never triggers. -
Asymmetric delete semantics — the plural branch unconditionally deletes
obj[k]and assigns the hydrated array (which may containundefinedentries if lookups fail), while the singular branch only replaces when hydration succeeds. This means a failed plural lookup silently loses the original IDs.
Suggested defensive fix
if (m[2]) {
- const hydrated = obj[k].map(function(id) {
- return self._lookupSubType(subType, type, id, root);
- });
- obj[self.pluralize(subType)] = hydrated || [];
- delete obj[k];
+ const ids = obj[k];
+ if (Array.isArray(ids)) {
+ const hydrated = ids.map(function(id) {
+ return self._lookupSubType(subType, type, id, root);
+ });
+ obj[self.pluralize(subType)] = hydrated;
+ delete obj[k];
+ }
} else {🤖 Prompt for AI Agents
In `@app/assets/javascripts/discourse/models/store.js.es6` around lines 196 - 208,
The plural `_ids` branch is not defensive: it calls obj[k].map without checking
obj[k], uses an unreachable `hydrated || []`, and unconditionally deletes obj[k]
even if lookups fail. Update the plural branch in the block that references
m[2], subType, type, root, self._lookupSubType and self.pluralize to first
verify Array.isArray(obj[k]); if true, map to hydrated items, filter out falsy
results, and only assign obj[self.pluralize(subType)] and delete obj[k] when the
filtered hydrated array has at least one valid element; otherwise leave obj[k]
unchanged (do not delete) to mirror the singular branch semantics and remove the
unnecessary `|| []` fallback.
| @@ -0,0 +1,34 @@ | |||
| class Admin::EmbeddableHostsController < Admin::AdminController | |||
|
|
|||
| before_filter :ensure_logged_in, :ensure_staff | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the Admin::AdminController file
fd -t f "*admin_controller*" --max-one-result | head -20Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 321
🏁 Script executed:
# Search for Admin::AdminController class definition
rg -n "class Admin::AdminController" --type rubyRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 184
🏁 Script executed:
# Check the embeddable_hosts_controller specifically
cat -n app/controllers/admin/embeddable_hosts_controller.rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 1145
🏁 Script executed:
# Check the Admin::AdminController for before_filter/before_action
cat -n app/controllers/admin/admin_controller.rb | head -50Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 324
🏁 Script executed:
# Search for all before_filter/before_action in admin_controller
rg -n "before_filter|before_action" app/controllers/admin/admin_controller.rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 155
🏁 Script executed:
# Check Rails version in the project
cat Gemfile | grep -i railsRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 963
Remove redundant before_filter declarations — they're already enforced by Admin::AdminController.
Admin::AdminController (lines 3-4) already defines before_filter :ensure_logged_in and before_filter :ensure_staff, making these filters in the child controller redundant. Additionally, before_filter is deprecated; use before_action if needed elsewhere. Delete lines 3 of this file.
🤖 Prompt for AI Agents
In `@app/controllers/admin/embeddable_hosts_controller.rb` at line 3, Remove the
redundant before_filter declaration in the controller: delete the line calling
"before_filter :ensure_logged_in, :ensure_staff" because Admin::AdminController
already defines those filters; if you do need to add filters elsewhere prefer
the modern "before_action" API instead of before_filter.
| def update | ||
| host = EmbeddableHost.where(id: params[:id]).first | ||
| save_host(host) | ||
| end |
There was a problem hiding this comment.
Missing nil guard — update will raise NoMethodError if the host doesn't exist.
EmbeddableHost.where(id: params[:id]).first returns nil when the record is not found. Passing nil into save_host will blow up on line 23 with NoMethodError: undefined method 'host=' for nil.
Use find to get a 404 via RecordNotFound, or add a nil check.
Proposed fix
def update
- host = EmbeddableHost.where(id: params[:id]).first
+ host = EmbeddableHost.find(params[:id])
save_host(host)
end🤖 Prompt for AI Agents
In `@app/controllers/admin/embeddable_hosts_controller.rb` around lines 9 - 12,
The update action calls EmbeddableHost.where(id: params[:id]).first and then
unconditionally passes the result to save_host, which will raise NoMethodError
if nil; change this to either use EmbeddableHost.find(params[:id]) so
ActiveRecord raises RecordNotFound (returning a 404) or add a nil guard: check
the host (result of EmbeddableHost.where(...).first) and render a 404 or
appropriate error response before calling save_host. Ensure you update the
update method and keep save_host unchanged.
| def destroy | ||
| host = EmbeddableHost.where(id: params[:id]).first | ||
| host.destroy | ||
| render json: success_json | ||
| end |
There was a problem hiding this comment.
Same nil guard issue in destroy — will 500 if the record doesn't exist.
host can be nil, causing NoMethodError on host.destroy.
Proposed fix
def destroy
- host = EmbeddableHost.where(id: params[:id]).first
+ host = EmbeddableHost.find(params[:id])
host.destroy
render json: success_json
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def destroy | |
| host = EmbeddableHost.where(id: params[:id]).first | |
| host.destroy | |
| render json: success_json | |
| end | |
| def destroy | |
| host = EmbeddableHost.find(params[:id]) | |
| host.destroy | |
| render json: success_json | |
| end |
🤖 Prompt for AI Agents
In `@app/controllers/admin/embeddable_hosts_controller.rb` around lines 14 - 18,
The destroy action calls host.destroy without guarding against a missing record,
which raises NoMethodError when EmbeddableHost.where(id: params[:id]).first
returns nil; update the destroy method (reference: destroy, EmbeddableHost,
params[:id], success_json) to check for a nil host (e.g., use find_by or the
existing query) and return an appropriate 404 or error response (render json
with an error or head :not_found) when host is nil, otherwise call destroy and
render success_json.
| def update | ||
| render_serialized(@embedding, EmbeddingSerializer, root: 'embedding', rest_serializer: true) | ||
| end |
There was a problem hiding this comment.
update action is a no-op — it never processes request params.
The action re-renders the current embedding state without reading or persisting any submitted data. If the intent is to update embedding-level settings in the future, this is a placeholder; if individual host CRUD is entirely handled via embeddable_hosts resource, this is just misleading. Consider either adding the update logic or making this action explicit about returning the current state (e.g., aliasing to show).
🤖 Prompt for AI Agents
In `@app/controllers/admin/embedding_controller.rb` around lines 9 - 11, The
update action in the controller (def update) is a no-op because it never
consumes params or persists changes; either implement proper update logic or
make it explicit that it simply returns the current resource. To fix: if update
should persist changes, read permitted params (e.g., embedding_params), call
`@embedding.update`!(embedding_params) (or update and handle errors), then
render_serialized(`@embedding`, EmbeddingSerializer, root: 'embedding',
rest_serializer: true); otherwise replace or alias update to the show action (or
return a 405/explicit message) so callers aren’t misled. Reference: the update
method, render_serialized call, EmbeddingSerializer, and the embeddable_hosts
resource mentioned in the comment.
| records = val.split("\n") | ||
| if records.present? | ||
| records.each do |h| | ||
| execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES ('#{h}', #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)" |
There was a problem hiding this comment.
SQL injection via unescaped string interpolation.
The host value h from the existing site setting is interpolated directly into the INSERT statement. A host value containing a single quote (e.g., a malformed entry) would break the query or allow SQL injection.
Proposed fix: use `quote` to escape the value
- execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES ('#{h}', #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)"
+ escaped_host = ActiveRecord::Base.connection.quote(h)
+ execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES (#{escaped_host}, #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES ('#{h}', #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)" | |
| escaped_host = ActiveRecord::Base.connection.quote(h) | |
| execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES (#{escaped_host}, #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)" |
🤖 Prompt for AI Agents
In `@db/migrate/20150818190757_create_embeddable_hosts.rb` at line 25, The INSERT
uses unescaped string interpolation of h causing SQL injection; change the raw
execute call in the migration to pass a properly quoted/escaped host value (use
the connection quote method, e.g. ActiveRecord::Base.connection.quote(h) or
connection.quote(h)) and interpolate that quoted value instead of h (keep
category_id as-is or bind it), so the execute statement uses the escaped host
string.
| Fabricator(:embeddable_host) do | ||
| host "eviltrout.com" | ||
| category | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Check if a category fabricator is defined anywhere else
echo "=== Searching for Fabricator(:category) definitions ==="
rg -n "Fabricator\(:category" --type ruby
# 2. Check if any specs still reference Fabricate(:category) or the removed variants
echo ""
echo "=== Searching for Fabricate(:category) usages ==="
rg -n "Fabricate\(:category\)" --type ruby | head -30
echo ""
echo "=== Searching for removed variant usages ==="
rg -n "Fabricate\(:(diff_category|happy_category|private_category)\)" --type ruby | head -20
# 3. Check if there's an embeddable_host fabricator file elsewhere
echo ""
echo "=== Searching for embeddable_host fabricator files ==="
fd "embeddable_host" --type f
# 4. Check the current state of the file
echo ""
echo "=== Current content of category_fabricator.rb ==="
cat spec/fabricators/category_fabricator.rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 3360
🏁 Script executed:
#!/bin/bash
echo "=== Content of embeddable_host_fabricator.rb ==="
cat spec/fabricators/embeddable_host_fabricator.rb
echo ""
echo "=== Checking if there are any other category-related fabricators ==="
rg -n "Fabricator\(" spec/fabricators/ --type ruby | grep -E "(category|embeddable_host)"Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 1253
Fabricator files have swapped content: reorganize to match filenames.
The category_fabricator.rb and embeddable_host_fabricator.rb files contain each other's content:
category_fabricator.rbcurrently holdsFabricator(:embeddable_host)embeddable_host_fabricator.rbcurrently holdsFabricator(:category),:diff_category,:happy_category, and:private_category
Move the category fabricators back to category_fabricator.rb and the embeddable_host fabricator to embeddable_host_fabricator.rb. While the code functions because the fabricators exist, this file organization violates naming conventions and will cause confusion during future maintenance.
🤖 Prompt for AI Agents
In `@spec/fabricators/category_fabricator.rb` around lines 1 - 4, The fabricator
definitions are swapped: move the Fabricator(:category) and its variants
(/:diff_category, :happy_category, :private_category/) into the file currently
containing Fabricator(:embeddable_host), and move the
Fabricator(:embeddable_host) (host "eviltrout.com", category) into the file
currently containing the category fabricators so each file contains the matching
Fabricator definitions (ensure Fabricator(:category),
Fabricator(:diff_category), Fabricator(:happy_category),
Fabricator(:private_category) live together and Fabricator(:embeddable_host) is
in its own embeddable_host file).
| Fabricator(:category) do | ||
| name { sequence(:name) { |n| "Amazing Category #{n}" } } | ||
| user | ||
| end | ||
|
|
||
| Fabricator(:diff_category, from: :category) do | ||
| name "Different Category" | ||
| user | ||
| end | ||
|
|
||
| Fabricator(:happy_category, from: :category) do | ||
| name 'Happy Category' | ||
| slug 'happy' | ||
| user | ||
| end | ||
|
|
||
| Fabricator(:private_category, from: :category) do | ||
| transient :group | ||
|
|
||
| name 'Private Category' | ||
| slug 'private' | ||
| user | ||
| after_build do |cat, transients| | ||
| cat.update!(read_restricted: true) | ||
| cat.category_groups.build(group_id: transients[:group].id, permission_type: CategoryGroup.permission_types[:full]) | ||
| end | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what's in category_fabricator.rb
echo "=== Contents of category_fabricator.rb ==="
fd 'category_fabricator.rb' --type f --exec cat {}
echo -e "\n=== Contents of embeddable_host_fabricator.rb ==="
fd 'embeddable_host_fabricator.rb' --type f --exec cat {}
echo -e "\n=== All Fabricator definitions (embeddable_host and category) ==="
rg -n 'Fabricator\(:(embeddable_host|category)' --type ruby -g '*fabricator*'Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 1071
Swap the contents of embeddable_host_fabricator.rb and category_fabricator.rb to align filenames with their fabricator definitions.
Currently, embeddable_host_fabricator.rb defines the category fabricators, while category_fabricator.rb defines the embeddable_host fabricator. This violates the convention where fabricator filenames match the models they fabricate and will confuse contributors trying to locate fabricators.
embeddable_host_fabricator.rbshould containFabricator(:embeddable_host)category_fabricator.rbshould containFabricator(:category)and its variants
🤖 Prompt for AI Agents
In `@spec/fabricators/embeddable_host_fabricator.rb` around lines 1 - 27, The
current file contains category fabricators (Fabricator(:category),
Fabricator(:diff_category), Fabricator(:happy_category),
Fabricator(:private_category)) but is named for embeddable_host; move the block
that defines Fabricator(:embeddable_host) from the other file into this file and
move the entire category fabricator block into the file that currently defines
embeddable_host so each file contains the fabricators matching its filename;
ensure you preserve transients and after_build hooks (e.g., the transient :group
and cat.update!/cat.category_groups.build logic) when relocating the
Fabricator(:private_category) and keep associations like user intact.
| describe EmbeddableHost do | ||
|
|
||
| it "trims http" do |
There was a problem hiding this comment.
RuboCop: extra blank lines at block body boundaries.
Static analysis flagged empty line violations at the beginning (Line 5) and end (Lines 38, 40) of block bodies.
Proposed fix
describe EmbeddableHost do
-
it "trims http" doAnd at the end of the file:
expect(EmbeddableHost.host_allowed?('http://discourse.org')).to eq(true)
end
-
end
-
end🧰 Tools
🪛 RuboCop (1.84.0)
[convention] 4-5: Extra empty line detected at block body beginning.
(Layout/EmptyLinesAroundBlockBody)
🤖 Prompt for AI Agents
In `@spec/models/embeddable_host_spec.rb` around lines 3 - 5, The spec contains
extra blank lines at block body boundaries; remove leading/trailing empty lines
inside the RSpec blocks (e.g., the describe EmbeddableHost block and individual
it examples like it "trims http") so there are no blank lines immediately after
a block opening or immediately before a block closing; update the spec examples
to start content on the first line of the block and end without a trailing blank
line.
| SiteSetting.embed_truncate = false | ||
| expect(topic.expandable_first_post?).to eq(false) | ||
| end | ||
| describe 'with an emeddable host' do |
There was a problem hiding this comment.
Typo: "emeddable" → "embeddable".
- describe 'with an emeddable host' do
+ describe 'with an embeddable host' do📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe 'with an emeddable host' do | |
| describe 'with an embeddable host' do |
🤖 Prompt for AI Agents
In `@spec/models/topic_spec.rb` at line 1407, Typo in the example/description
string: change the RSpec describe block title "with an emeddable host" to the
correct spelling "with an embeddable host" (update the describe 'with an
emeddable host' line in the spec to describe 'with an embeddable host').
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Tests