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

V0 11 dev performance #1431

Merged
merged 9 commits into from
Jan 16, 2024
14 changes: 5 additions & 9 deletions lib/jsonapi/link_builder.rb
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) }"
lgebhardt marked this conversation as resolved.
Show resolved Hide resolved
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
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
bf4 marked this conversation as resolved.
Show resolved Hide resolved

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
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? }
lgebhardt marked this conversation as resolved.
Show resolved Hide resolved
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
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|
lgebhardt marked this conversation as resolved.
Show resolved Hide resolved
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|
lgebhardt marked this conversation as resolved.
Show resolved Hide resolved
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
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