Skip to content

V0 11 dev performance #1431

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Jan 16, 2024
Merged

V0 11 dev performance #1431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
867af02
Reduce number of string allocations in LinkBuilder
lgebhardt Jan 15, 2024
755142c
Consistently access `include_related`
lgebhardt Jan 15, 2024
3121ccb
Remove unused class variable
lgebhardt Jan 15, 2024
a83920e
Cache `id` after retrieving it from the model
lgebhardt Jan 15, 2024
f7c58cc
Cache `module_path`
lgebhardt Jan 15, 2024
aad31ba
Cache resource_klass_for and resource_type_for
lgebhardt Jan 15, 2024
5cd779c
Remove no longer used method _setup_relationship
lgebhardt Jan 15, 2024
c4eedf2
Delete nil values without creating a new object
lgebhardt Jan 15, 2024
d7c78a2
Rework resource naming for method caches
lgebhardt Jan 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions lib/jsonapi/link_builder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ def query_link(query_params)

def relationships_related_link(source, relationship, query_params = {})
if relationship._routed
url = "#{ self_link(source) }/#{ route_for_relationship(relationship) }"
url = "#{ url }?#{ query_params.to_query }" if query_params.present?
url = +"#{ self_link(source) }/#{ route_for_relationship(relationship) }"
url << "?#{ query_params.to_query }" if query_params.present?
url
else
if JSONAPI.configuration.warn_on_missing_routes && !relationship._warned_missing_route
Expand Down Expand Up @@ -129,18 +129,14 @@ def resources_path(source_klass)
@_resources_path[source_klass] ||= formatted_module_path_from_class(source_klass) + format_route(source_klass._type.to_s)
end

def resource_path(source)
def resource_url(source)
if source.class.singleton?
resources_path(source.class)
"#{ base_url }#{ engine_mount_point }#{ resources_path(source.class) }"
else
"#{resources_path(source.class)}/#{source.id}"
"#{ base_url }#{ engine_mount_point }#{resources_path(source.class)}/#{source.id}"
end
end

def resource_url(source)
"#{ base_url }#{ engine_mount_point }#{ resource_path(source) }"
end

def route_for_relationship(relationship)
format_route(relationship.name)
end
Expand Down
95 changes: 57 additions & 38 deletions lib/jsonapi/resource_common.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def _model
end

def id
_model.public_send(self.class._primary_key)
@id ||= _model.public_send(self.class._primary_key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Off topic-- we'll want to handle composite primary keys here

Copy link
Member Author

Choose a reason for hiding this comment

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

That's an interesting point. I don't see anything in the spec to provide support for a multi attribute composite key in the JSONAPI section on identification. We could attempt to support this in JR by combining the composite key components, but I suspect it's going to run into the same issues I encountered trying to find a way to support an alternate public key field (one different from the primary key in the database). It was a harder problem than I initially thought it would be.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I figure someone could also just override this method. Since keys need to be consistently ordered, a custom synthetic key would be fine and keep the risk of a spec error off of jsonapi.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's the way I would handle it too. To work with rails we would need an identity class to convert the composite string keys to an array and back. It would an interesting thing to test.

end

def identity
Expand Down Expand Up @@ -510,7 +510,10 @@ def inherited(subclass)
subclass._routed = false
subclass._warned_missing_route = false

subclass._clear_cached_attribute_options
subclass._attribute_options_cache = {}
subclass._model_class_to_resource_type_cache = {}
subclass._resource_type_to_class_cache = {}

subclass._clear_fields_cache

subclass._resource_retrieval_strategy_loaded = @_resource_retrieval_strategy_loaded
Expand All @@ -533,15 +536,19 @@ def rebuild_relationships(relationships)
end

def resource_klass_for(type)
@_resource_type_to_class_cache ||= {}
type = type.underscore
type_with_module = type.start_with?(module_path) ? type : module_path + type

resource_name = _resource_name_from_type(type_with_module)
resource = resource_name.safe_constantize if resource_name
if resource.nil?
fail NameError, "JSONAPI: Could not find resource '#{type}'. (Class #{resource_name} not found)"
@_resource_type_to_class_cache.fetch(type) do
type_with_module = type.start_with?(module_path) ? type : module_path + type

resource_name = _resource_name_from_type(type_with_module)
resource_klass = resource_name.safe_constantize if resource_name
if resource_klass.nil?
fail NameError, "JSONAPI: Could not find resource '#{type}'. (Class #{resource_name} not found)"
end
@_resource_type_to_class_cache[type] = resource_klass
end
resource
end

def resource_klass_for_model(model)
Expand All @@ -553,17 +560,33 @@ def _resource_name_from_type(type)
end

def resource_type_for(model)
model_name = model.class.to_s.underscore
if _model_hints[model_name]
_model_hints[model_name]
else
model_name.rpartition('/').last
@_model_class_to_resource_type_cache.fetch(model.class) do
model_name = model.class.name.underscore

resource_type = if _model_hints[model_name]
_model_hints[model_name]
else
model_name.rpartition('/').last
end

@_model_class_to_resource_type_cache[model.class] = resource_type
end
end

attr_accessor :_attributes, :_relationships, :_type, :_model_hints, :_routed, :_warned_missing_route,
attr_accessor :_attributes,
:_relationships,
:_type,
:_model_hints,
:_routed,
:_warned_missing_route,
:_resource_retrieval_strategy_loaded
attr_writer :_allowed_filters, :_paginator, :_allowed_sort

attr_writer :_allowed_filters,
:_paginator,
:_allowed_sort,
:_model_class_to_resource_type_cache,
:_resource_type_to_class_cache,
:_attribute_options_cache

def create(context)
new(create_model, context)
Expand All @@ -590,7 +613,7 @@ def attributes(*attrs)
end

def attribute(attribute_name, options = {})
_clear_cached_attribute_options
_clear_attribute_options_cache
_clear_fields_cache

attr = attribute_name.to_sym
Expand Down Expand Up @@ -903,7 +926,7 @@ def verify_relationship_filter(filter, raw, _context = nil)

# quasi private class methods
def _attribute_options(attr)
@_cached_attribute_options[attr] ||= default_attribute_options.merge(@_attributes[attr])
@_attribute_options_cache[attr] ||= default_attribute_options.merge(@_attributes[attr])
end

def _attribute_delegated_name(attr)
Expand All @@ -915,11 +938,11 @@ def _has_attribute?(attr)
end

def _updatable_attributes
_attributes.map { |key, options| key unless options[:readonly] }.compact
_attributes.map { |key, options| key unless options[:readonly] }.delete_if {|v| v.nil? }
end

def _updatable_relationships
@_relationships.map { |key, relationship| key unless relationship.readonly? }.compact
@_relationships.map { |key, relationship| key unless relationship.readonly? }.delete_if {|v| v.nil? }
end

def _relationship(type)
Expand Down Expand Up @@ -1132,11 +1155,11 @@ def _has_sort?(sorting)
end

def module_path
if name == 'JSONAPI::Resource'
''
else
name =~ /::[^:]+\Z/ ? ($`.freeze.gsub('::', '/') + '/').underscore : ''
end
@module_path ||= if name == 'JSONAPI::Resource'
''
else
name =~ /::[^:]+\Z/ ? ($`.freeze.gsub('::', '/') + '/').underscore : ''
end
end

def default_sort
Expand Down Expand Up @@ -1169,18 +1192,6 @@ def _add_relationship(klass, *attrs)
end
end

def _setup_relationship(klass, *attrs)
_clear_fields_cache

options = attrs.extract_options!
options[:parent_resource] = self

relationship_name = attrs[0].to_sym
check_duplicate_relationship_name(relationship_name)

define_relationship_methods(relationship_name.to_sym, klass, options)
end

# ResourceBuilder methods
def define_relationship_methods(relationship_name, relationship_klass, options)
relationship = register_relationship(
Expand Down Expand Up @@ -1214,8 +1225,16 @@ def register_relationship(name, relationship_object)
@_relationships[name] = relationship_object
end

def _clear_cached_attribute_options
@_cached_attribute_options = {}
def _clear_attribute_options_cache
@_attribute_options_cache&.clear
end

def _clear_model_to_resource_type_cache
@_model_class_to_resource_type_cache&.clear
end

def _clear_resource_type_to_klass_cache
@_resource_type_to_class_cache&.clear
end

def _clear_fields_cache
Expand Down
4 changes: 2 additions & 2 deletions lib/jsonapi/resource_serializer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ def links_hash(source)
if !links.key?('self') && !source.class.exclude_link?(:self)
links['self'] = link_builder.self_link(source)
end
links.compact
links.delete_if {|k,v| v.nil? }
end

def custom_links_hash(source)
Expand Down Expand Up @@ -340,7 +340,7 @@ def default_relationship_links(source, relationship)
links = {}
links['self'] = self_link(source, relationship) unless relationship.exclude_link?(:self)
links['related'] = related_link(source, relationship) unless relationship.exclude_link?(:related)
links.compact
links.delete_if {|k,v| v.nil? }
end

def to_many_linkage(rids)
Expand Down
5 changes: 2 additions & 3 deletions lib/jsonapi/resource_tree.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,13 @@ def add_resource(resource, include_related)
private

def init_included_relationships(fragment, include_related)
include_related && include_related.each_key do |relationship_name|
include_related&.each_key do |relationship_name|
fragment.initialize_related(relationship_name)
end
end

def load_included(resource_klass, source_resource_tree, include_related, options)
include_related.try(:each_key) do |key|
include_related&.each_key do |key|
relationship = resource_klass._relationship(key)
relationship_name = relationship.name.to_sym

Expand Down Expand Up @@ -159,7 +159,6 @@ def initialize(parent_relationship, source_resource_tree)
@related_resource_trees ||= {}

@parent_relationship = parent_relationship
@parent_relationship_name = parent_relationship.name.to_sym
@source_resource_tree = source_resource_tree
end

Expand Down
4 changes: 4 additions & 0 deletions test/controllers/controller_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4028,6 +4028,8 @@ def test_immutable_update_not_supported

class Api::V7::ClientsControllerTest < ActionController::TestCase
def test_get_namespaced_model_not_matching_resource_using_model_hint
Api::V7::ClientResource._clear_model_to_resource_type_cache
Api::V7::ClientResource._clear_resource_type_to_klass_cache
assert_cacheable_get :index
assert_response :success
assert_equal 'clients', json_response['data'][0]['type']
Expand All @@ -4037,6 +4039,8 @@ def test_get_namespaced_model_not_matching_resource_using_model_hint

def test_get_namespaced_model_not_matching_resource_not_using_model_hint
Api::V7::ClientResource._model_hints.delete('api/v7/customer')
Api::V7::ClientResource._clear_model_to_resource_type_cache
Api::V7::ClientResource._clear_resource_type_to_klass_cache
assert_cacheable_get :index
assert_response :success
assert_equal 'customers', json_response['data'][0]['type']
Expand Down