Skip to content

Commit

Permalink
Express ids as uncompressed strings
Browse files Browse the repository at this point in the history
Support for compressed ids was added and enabled by default (rendering
all ids in their compressed form) with the thought of killing two
birds with one stone:

1. javascript doesn't support very large numbers
2. we wanted to add support for compressed ids

It turns out that (2) wasn't solving any problems in UI land
anymore. With the added complexity and emerging issues¹ that came as a
result of this change, it seemed better to remove the compressed id
support and simply render all ids as strings instead, which is
sufficient to solve the javascript large numbers issue.

Support for incoming compressed ids in URLs has been left, since this
has already been incorporated into the Fine release.

1. See ManageIQ#49,
ManageIQ/manageiq#15658
  • Loading branch information
imtayadeway committed Sep 18, 2017
1 parent 6a83d2d commit 2089731
Show file tree
Hide file tree
Showing 62 changed files with 714 additions and 731 deletions.
2 changes: 1 addition & 1 deletion app/controllers/api/base_controller/action.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def api_action(type, id, options = {})

result = yield(klass) if block_given?

add_href_to_result(result, type, compress_if_numeric_id(id)) unless options[:skip_href]
add_href_to_result(result, type, id) unless options[:skip_href]
log_result(result)
result
end
Expand Down
6 changes: 3 additions & 3 deletions app/controllers/api/base_controller/generic.rb
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ def delete_resource_action(klass, type, id)
rescue => err
action_result(false, "#{err} - #{resource.errors.full_messages.join(', ')}")
end
add_href_to_result(result, type, ApplicationRecord.compress_id(id))
add_href_to_result(result, type, id)
log_result(result)
result
end
Expand All @@ -204,7 +204,7 @@ def invoke_custom_action(type, resource, action, data)
rescue => err
action_result(false, err.to_s)
end
add_href_to_result(result, type, resource.compressed_id)
add_href_to_result(result, type, resource.id)
log_result(result)
result
end
Expand All @@ -218,7 +218,7 @@ def invoke_custom_action_with_dialog(type, resource, action, data, custom_button
rescue => err
action_result(false, err.to_s)
end
add_href_to_result(result, type, resource.compressed_id)
add_href_to_result(result, type, resource.id)
log_result(result)
result
end
Expand Down
6 changes: 3 additions & 3 deletions app/controllers/api/base_controller/normalizer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ def normalize_hash(type, obj, opts = {})
attrs = normalize_select_attributes(obj, opts)
result = {}

href = new_href(compress_path(type), compress_if_numeric_id(obj["id"]), obj["href"])
href = new_href(type, obj["id"], obj["href"])
if href.present?
result["href"] = href
attrs -= ["href"]
Expand All @@ -32,7 +32,7 @@ def normalize_attr(attr, value)
elsif value.respond_to?(:attributes) || value.respond_to?(:keys)
normalize_hash(attr, value)
elsif attr == "id" || attr.to_s.ends_with?("_id")
ApplicationRecord.compress_id(value)
value.to_s
elsif Api.time_attribute?(attr)
normalize_time(value)
elsif Api.url_attribute?(attr)
Expand Down Expand Up @@ -78,7 +78,7 @@ def normalize_href(type, value)
end

def subcollection_href(type, value)
normalize_url("#{@req.collection}/#{ApplicationRecord.compress_id(@req.c_id)}/#{type}/#{value}")
normalize_url("#{@req.collection}/#{@req.c_id}/#{type}/#{value}")
end

def collection_href(type, value)
Expand Down
20 changes: 2 additions & 18 deletions app/controllers/api/base_controller/renderer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ def collection_to_jbuilder(type, reftype, resources, opts = {})
if opts[:expand_resources]
add_hash json, resource_to_jbuilder(type, reftype, resource, opts).attributes!
else
json.href normalize_href(compress_path(reftype), ApplicationRecord.compress_id(resource["id"]))
json.href normalize_href(reftype, resource["id"])
end
end
end
Expand Down Expand Up @@ -405,7 +405,7 @@ def expand_subcollection(json, sc, sctype, subresources)
if @req.expand?(sc) || scr["id"].nil?
add_child js, normalize_hash(sctype, scr)
else
js.child! { |jsc| jsc.href normalize_href(sctype, ApplicationRecord.compress_id(scr["id"])) }
js.child! { |jsc| jsc.href normalize_href(sctype, scr["id"]) }
end
end
end
Expand Down Expand Up @@ -482,22 +482,6 @@ def render_options(resource, data = {})
klass = collection_class(resource)
render :json => OptionsSerializer.new(klass, data).serialize
end

def compress_path(path)
if path.to_s.split("/").size > 1
path.to_s.split("/").tap { |e| e[1] = ApplicationRecord.compress_id(e[1]) if e[1] =~ /\A[0-9]+\z/ }.join("/")
else
path
end
end

def compress_if_numeric_id(id)
if id.to_s =~ /\A[0-9]+\z/
ApplicationRecord.compress_id(id)
else
id
end
end
end
end
end
14 changes: 7 additions & 7 deletions app/controllers/api/base_controller/results.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ def action_result(success, message = nil, options = {})
res[:result] = options[:result] unless options[:result].nil?
add_task_to_result(res, options[:task_id]) if options[:task_id].present?
add_tasks_to_result(res, options[:task_ids]) if options[:task_ids].present?
add_parent_href_to_result(res, ApplicationRecord.compress_id(options[:parent_id])) if options[:parent_id].present?
add_parent_href_to_result(res, options[:parent_id]) if options[:parent_id].present?
res
end

Expand All @@ -20,27 +20,27 @@ def add_href_to_result(hash, type, id)

def add_parent_href_to_result(hash, parent_id = nil)
return if hash[:href].present?
hash[:href] = "#{@req.api_prefix}/#{@req.collection}/#{parent_id ? parent_id : ApplicationRecord.compress_id(@req.c_id)}"
hash[:href] = "#{@req.api_prefix}/#{@req.collection}/#{parent_id ? parent_id : @req.c_id}"
hash
end

def add_task_to_result(hash, task_id)
hash[:task_id] = ApplicationRecord.compress_id(task_id)
hash[:task_href] = task_href(ApplicationRecord.compress_id(task_id))
hash[:task_id] = task_id.to_s
hash[:task_href] = task_href(task_id)
hash
end

def add_tasks_to_result(hash, task_ids)
add_task_to_result(hash, task_ids.first)
hash[:tasks] = task_ids.collect do |task_id|
{ :id => ApplicationRecord.compress_id(task_id), :href => task_href(ApplicationRecord.compress_id(task_id)) }
{ :id => task_id.to_s, :href => task_href(task_id) }
end
end

def add_tag_to_result(hash, tag_spec)
hash[:tag_category] = tag_spec[:category] if tag_spec[:category].present?
hash[:tag_name] = tag_spec[:name] if tag_spec[:name].present?
hash[:tag_href] = "#{@req.api_prefix}/tags/#{ApplicationRecord.compress_id(tag_spec[:id])}" if tag_spec[:id].present?
hash[:tag_href] = "#{@req.api_prefix}/tags/#{tag_spec[:id]}" if tag_spec[:id].present?
hash
end

Expand All @@ -52,7 +52,7 @@ def add_subcollection_resource_to_result(hash, ctype, object)
return hash if object.blank?
ctype_pref = ctype.to_s.singularize
hash["#{ctype_pref}_id".to_sym] = object.id
hash["#{ctype_pref}_href".to_sym] = "#{@req.api_prefix}/#{ctype}/#{object.compressed_id}"
hash["#{ctype_pref}_href".to_sym] = "#{@req.api_prefix}/#{ctype}/#{object.id}"
hash
end

Expand Down
6 changes: 3 additions & 3 deletions app/controllers/api/reports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def run_report_result(success, message = nil, options = {})
res = {:success => success}
res[:message] = message if message.present?
add_parent_href_to_result(res)
add_report_result_to_result(res, ApplicationRecord.compress_id(options[:report_result_id])) if options[:report_result_id].present?
add_report_result_to_result(res, options[:report_result_id]) if options[:report_result_id].present?
add_task_to_result(res, options[:task_id]) if options[:task_id].present?
res
end
Expand Down Expand Up @@ -60,8 +60,8 @@ def schedule_reports(report, type, id, data)
desc = "scheduling of report #{report.id}"
schedule = report.add_schedule fetch_schedule_data(data)
res = action_result(true, desc)
add_report_schedule_to_result(res, schedule.compressed_id, report.compressed_id)
add_href_to_result(res, type, ApplicationRecord.compress_id(id))
add_report_schedule_to_result(res, schedule.id, report.id)
add_href_to_result(res, type, id)
res
rescue => err
action_result(false, err.to_s)
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/api/services_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ def invoke_reconfigure_dialog(type, svc, data = {})
rescue => err
action_result(false, err.to_s)
end
add_href_to_result(result, type, svc.compressed_id)
add_href_to_result(result, type, svc.id)
log_result(result)
result
end
Expand Down
2 changes: 1 addition & 1 deletion spec/requests/actions_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@

expect(response).to have_http_status(:ok)

action_id = ApplicationRecord.uncompress_id(response.parsed_body["results"].first["id"])
action_id = response.parsed_body["results"].first["id"]

expect(MiqAction.exists?(action_id)).to be_truthy
end
Expand Down
26 changes: 13 additions & 13 deletions spec/requests/alert_definitions_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@
"subcount" => 2,
"resources" => a_collection_containing_exactly(
{
"href" => api_alert_definition_url(nil, alert_definitions[0].compressed_id)
"href" => api_alert_definition_url(nil, alert_definitions[0])
},
{
"href" => api_alert_definition_url(nil, alert_definitions[1].compressed_id)
"href" => api_alert_definition_url(nil, alert_definitions[1])
}
)
)
Expand All @@ -49,8 +49,8 @@
get(api_alert_definition_url(nil, alert_definition))
expect(response).to have_http_status(:ok)
expect(response.parsed_body).to include(
"href" => api_alert_definition_url(nil, alert_definition.compressed_id),
"id" => alert_definition.compressed_id,
"href" => api_alert_definition_url(nil, alert_definition),
"id" => alert_definition.id.to_s,
"description" => alert_definition.description,
"guid" => alert_definition.guid,
"expression" => {"exp" => {"=" => {"field" => "Vm-name", "value" => "foo"}}, "context_type" => nil}
Expand Down Expand Up @@ -78,7 +78,7 @@
api_basic_authorize collection_action_identifier(:alert_definitions, :create)
post(api_alert_definitions_url, :params => sample_alert_definition)
expect(response).to have_http_status(:ok)
alert_definition = MiqAlert.find(ApplicationRecord.uncompress_id(response.parsed_body["results"].first["id"]))
alert_definition = MiqAlert.find(response.parsed_body["results"].first["id"])
expect(alert_definition).to be_truthy
expect(alert_definition.expression.class).to eq(MiqExpression)
expect(alert_definition.expression.exp).to eq(sample_alert_definition["expression"])
Expand All @@ -98,7 +98,7 @@
expect(response).to have_http_status(:ok)
expect_single_action_result(:success => true,
:message => "alert_definitions id: #{alert_definition.id} deleting",
:href => api_alert_definition_url(nil, alert_definition.compressed_id))
:href => api_alert_definition_url(nil, alert_definition))
end

it "deletes an alert definition via DELETE" do
Expand Down Expand Up @@ -196,8 +196,8 @@
expect_query_result(:alert_definition_profiles, 2, 2)
expect_result_resources_to_include_hrefs(
"resources",
[api_alert_definition_profile_url(nil, alert_definition_profiles.first.compressed_id),
api_alert_definition_profile_url(nil, alert_definition_profiles.second.compressed_id)]
[api_alert_definition_profile_url(nil, alert_definition_profiles.first),
api_alert_definition_profile_url(nil, alert_definition_profiles.second)]
)
end

Expand All @@ -208,7 +208,7 @@

expect(response).to have_http_status(:ok)
expect(response.parsed_body).to include(
"href" => api_alert_definition_profile_url(nil, alert_definition_profile.compressed_id),
"href" => api_alert_definition_profile_url(nil, alert_definition_profile),
"description" => alert_definition_profile.description,
"guid" => alert_definition_profile.guid
)
Expand All @@ -224,8 +224,8 @@
expect(response).to have_http_status(:ok)
expect_result_resources_to_include_hrefs(
"resources",
[api_alert_definition_profile_alert_definitions_url(nil, alert_definition_profile.compressed_id, alert_definitions.first.compressed_id),
api_alert_definition_profile_alert_definitions_url(nil, alert_definition_profile.compressed_id, alert_definitions.first.compressed_id)]
[api_alert_definition_profile_alert_definitions_url(nil, alert_definition_profile.id.to_s, alert_definitions.first),
api_alert_definition_profile_alert_definitions_url(nil, alert_definition_profile.id.to_s, alert_definitions.first)]
)
end

Expand Down Expand Up @@ -255,7 +255,7 @@
post(api_alert_definition_profiles_url, :params => sample_alert_definition_profile)

expect(response).to have_http_status(:ok)
id = ApplicationRecord.uncompress_id(response.parsed_body["results"].first["id"])
id = response.parsed_body["results"].first["id"]
alert_definition_profile = MiqAlertSet.find(id)
expect(alert_definition_profile).to be_truthy
expect(response.parsed_body["results"].first).to include(
Expand All @@ -272,7 +272,7 @@
expect(response).to have_http_status(:ok)
expect_single_action_result(:success => true,
:message => "alert_definition_profiles id: #{alert_definition_profile.id} deleting",
:href => api_alert_definition_profile_url(nil, alert_definition_profile.compressed_id))
:href => api_alert_definition_profile_url(nil, alert_definition_profile))
end

it "deletes an alert definition profile via DELETE" do
Expand Down
24 changes: 12 additions & 12 deletions spec/requests/alerts_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@
"subcount" => 2,
"resources" => [
{
"href" => api_alert_url(nil, alert_statuses[0].compressed_id)
"href" => api_alert_url(nil, alert_statuses[0])
},
{
"href" => api_alert_url(nil, alert_statuses[1].compressed_id)
"href" => api_alert_url(nil, alert_statuses[1])
}
]
)
Expand All @@ -38,8 +38,8 @@
get(api_alert_url(nil, alert_status))
expect(response).to have_http_status(:ok)
expect(response.parsed_body).to include(
"href" => api_alert_url(nil, alert_status.compressed_id),
"id" => alert_status.compressed_id
"href" => api_alert_url(nil, alert_status),
"id" => alert_status.id.to_s
)
end

Expand All @@ -49,7 +49,7 @@
let(:expected_assignee) do
{
'results' => a_collection_containing_exactly(
a_hash_including("assignee_id" => assignee.compressed_id)
a_hash_including("assignee_id" => assignee.id.to_s)
)
}
end
Expand Down Expand Up @@ -80,7 +80,7 @@
"subcount" => 1,
"resources" => [
{
"href" => api_alert_alert_action_url(nil, alert.compressed_id, alert_action.compressed_id)
"href" => api_alert_alert_action_url(nil, alert.id.to_s, alert_action)
}
]
)
Expand Down Expand Up @@ -126,7 +126,7 @@
expect(response).to have_http_status(:ok)
expected = {
"results" => [
a_hash_including(attributes.merge("user_id" => User.current_user.compressed_id))
a_hash_including(attributes.merge("user_id" => User.current_user.id.to_s))
]
}
expect(response.parsed_body).to include(expected)
Expand All @@ -136,7 +136,7 @@
it "create an assignment alert action reference by id" do
attributes = {
"action_type" => "assign",
"assignee" => { "id" => assignee.compressed_id }
"assignee" => { "id" => assignee.id.to_s }
}
api_basic_authorize subcollection_action_identifier(:alerts, :alert_actions, :create, :post)
post(api_alert_alert_actions_url(nil, alert), :params => attributes)
Expand All @@ -147,7 +147,7 @@
it "create an assignment alert action reference by href" do
attributes = {
"action_type" => "assign",
"assignee" => { "href" => api_user_url(nil, assignee.compressed_id) }
"assignee" => { "href" => api_user_url(nil, assignee) }
}
api_basic_authorize subcollection_action_identifier(:alerts, :alert_actions, :create, :post)
post(api_alert_alert_actions_url(nil, alert), :params => attributes)
Expand Down Expand Up @@ -178,10 +178,10 @@
get(api_alert_alert_action_url(nil, alert, alert_action))
expect(response).to have_http_status(:ok)
expect(response.parsed_body).to include(
"href" => api_alert_alert_action_url(nil, alert.compressed_id, alert_action.compressed_id),
"id" => alert_action.compressed_id,
"href" => api_alert_alert_action_url(nil, alert.id.to_s, alert_action),
"id" => alert_action.id.to_s,
"action_type" => alert_action.action_type,
"user_id" => user.compressed_id,
"user_id" => user.id.to_s,
"comment" => alert_action.comment,
)
end
Expand Down
4 changes: 2 additions & 2 deletions spec/requests/authentications_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
'count' => 1,
'subcount' => 1,
'name' => 'authentications',
'resources' => [hash_including('href' => api_authentication_url(nil, auth.compressed_id))]
'resources' => [hash_including('href' => api_authentication_url(nil, auth))]
}
expect(response.parsed_body).to include(expected)
expect(response).to have_http_status(:ok)
Expand All @@ -36,7 +36,7 @@
get(api_authentication_url(nil, auth))

expected = {
'href' => api_authentication_url(nil, auth.compressed_id)
'href' => api_authentication_url(nil, auth)
}
expect(response.parsed_body).to include(expected)
expect(response).to have_http_status(:ok)
Expand Down
2 changes: 1 addition & 1 deletion spec/requests/automate_domains_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
expect_single_action_result(
:success => true,
:message => a_string_matching(/Refreshing Automate Domain .* from git repository/),
:href => api_automate_domain_url(nil, git_domain.compressed_id)
:href => api_automate_domain_url(nil, git_domain)
)
end

Expand Down
Loading

0 comments on commit 2089731

Please sign in to comment.