V0_11 dev #1411
Draft
V0_11 dev #1411
+6,828
−3,329
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lgebhardt
force-pushed
the
v0-11-dev
branch
from
a2fc02f
to
01ba9a0
Compare
bf4
reviewed
Oct 4, 2023
| # 'JSONAPI::ActiveRelationRetrievalV10' | ||
| # :none | ||
| # :self | ||
| self.default_resource_retrieval_strategy = 'JSONAPI::ActiveRelationRetrieval' |
bf4
reviewed
Oct 4, 2023
| # For 'JSONAPI::ActiveRelationRetrievalV10': use a related resource's `records` when performing joins. | ||
| # This setting allows included resources to account for permission scopes. It can be overridden explicitly per | ||
| # relationship. Furthermore, specifying a `relation_name` on a relationship will cause this setting to be ignored. | ||
| self.use_related_resource_records_for_joins = true |
There was a problem hiding this comment.
right now only changes behavior in ActiveRelationRetrievalV10
12 tasks
krasnoukhov
reviewed
Nov 3, 2023
|
|
||
| def find_related_fragments_from_inverse(source, source_relationship, options, connect_source_identity) | ||
| relationship = source_relationship.resource_klass._relationship(source_relationship.inverse_relationship) | ||
| raise "missing inverse relationship" unless relationship.present? |
There was a problem hiding this comment.
It would be nice to be more specific in this exception, would be easier for people to identify problematic relationship
krasnoukhov
reviewed
Nov 3, 2023
| if source_resource | ||
| source_resource.add_related_identity(source_relationship.name, related_resource.identity) | ||
| fragment.add_related_from(source_resource.identity) | ||
| fragment.add_related_identity(source_relationship.inverse_relationship, source_resource.identity) |
There was a problem hiding this comment.
It's possible that there's no inverse relationship, in this case this fails. Would it be ok to wrap this in a conditional to not do this if there's no inverse?
There was a problem hiding this comment.
I tried adding this conditional while integrating this in one of our apps and it worked just fine, everything passes
There was a problem hiding this comment.
I think that's a good idea. In order to provide clarity for the behavior we probably should log why it's being skipped (in dev mode) and allow a flag is set on the source_relationship to mute this message. I'll take a look at it.
There was a problem hiding this comment.
Hey @lgebhardt, were you able to look at this? This is the only thing that is preventing us from adopting further betas
There was a problem hiding this comment.
@krasnoukhov I haven't tackled this yet, but I will get to it very soon. I have made some progress on the rake task to detect missing inverse relationships, and should be able to get PRs out for both of these soon.
There was a problem hiding this comment.
Hey @lgebhardt, any progress here?
There was a problem hiding this comment.
@krasnoukhov Sorry to long wait. I had a project that took much more time than expected and it pulled me away from JR for the last 3 weeks. I'm getting back into it now and hope to have a new beta out soon.
bf4
reviewed
Dec 24, 2023
- also add dependabot workflow - also standardize rails/ruby version quoting/ordering
* refactor: easily quote table/column * refactor: extract table name when missing * fix: Reliably quote columns/tables * refactor: putting quoting methods together * Handle special case of * - tests * fix: hack mysql test query comparison
* fix: test the adapter-specific query ordering * test: make order independent * test: sort order-dependent response * test: skip failing mysql tests as ok for now
* Restore previous include directives behavior * Default sort use _primary_key * Remove support for pluck attributes * Pass relationship instead of relationship name * Update copyright date * Ignore docker-compose override files * add _relation_name method * Rework resource class to support using modules for retrieving resources by way of a `resource_retrieval_strategy` Removes BasicResource class and replaces ActiveRelationResource with a module * Use `_relationship` helper method * Add ActiveRelationRetrieval Allows retrieval of resources by querying the primary table and joining the source table - the opposite of the v10 version * Skip extra pluck queries when not caching a resource * Test Cleanup * Adjust tested query counts based on default_resource_retrieval_strategy * create_implicit_polymorphic_type_relationships * Add ActiveRelationRetrievalV09 * Move resource down in the load order * Use underscore instead of downcase * Refactor Resource to load retrieval strategy as class loads * Simplify loading resource retrieval strategy modules Add SimpleResource that does not load a resource retrieval strategy module * Remove no longer need deferred_relationship code * Add warning about potentially unused `records_for_populate` * Rework loading the resource_retrieval_strategy to fix issue in real projects * Use SortedSets for resource_identities * Add sorted_set gem * Remove rails 5 support
* fix: warnings * fix: warning * fix: warnings
* Restore `use_related_resource_records_for_joins` for v0_10 * Handle nil actual hashes * Add back join_options for v10 compatibility * Test JoinManager not JoinManagerV10 * Use sql_for_compare to account for different sql dialect quoating
handle gems like GraphQL which override `include?` ``` rake aborted! ArgumentError: wrong number of arguments (given 1, expected 3) gems/graphql-2.0.13/lib/graphql/schema/directive.rb:58:in `include?' ```
* Cleanup table definitions for Rails 7.1 * Test helper move require 'rails/test_help' * Test helper add `config.hosts` * Update test matrix to add rails 7.1 and remove ruby 2.6 Note: ruby 2.7 is also EOL, but I'm choosing to continue testing 2.7 for now
#1421) * Namespace references to Rails using `::Rails` to avoid conflicts with other gems * Use uppercase JSONAPI module name
* Make SortedSet for identity arrays optional * Fix tests to use sort_related_identities_by_primary_key option override * Keep SortedSet as a development dependency, unless required * Remove sorted_set dependency * Add better messaging about using SortedSet * Clarify setting sort_criteria for includes vs. related resources
Removes the need to allocate a new array for every comparison
add tests for ResourceIdentity, including that comparison does not allocate memory
* Reduce number of string allocations in LinkBuilder * Consistently access `include_related` * Remove unused class variable * Cache `id` after retrieving it from the model * Cache `module_path` * Cache resource_klass_for and resource_type_for * Remove no longer used method _setup_relationship * Delete nil values without creating a new object * Rework resource naming for method caches
* fix: check if relation retrieval in included via included_modules * require 'jsonapi/relation_retrieval' * feat: raise when cannot include different retrieval strategy * test: multiple retrieval strategies --------- Co-authored-by: lgebhardt <[email protected]>
* refactor: lookup polymorphic types only once * refactor: polymorphic lookups to utility module * refactor: separate polymorphic functions * Refactor into PolymorphicTypesLookup --------- Co-authored-by: lgebhardt <[email protected]>
* feat: teach JR in tests to parse the response * use response.parsed_body instead of JSON.parse when evaluating responses in tests --------- Co-authored-by: lgebhardt <[email protected]>
* chore(deprecation): test_fixture= has been deprecated in favor of tests_fixtures= * chore(deprecations): ActiveSupport::Deprecation.silenced ``` ActiveSupport::Deprecation is deprecated and will be removed from Rails (use Rails.application.deprecators.silenced= instead) ``` * chore(deprecation): prefer ActiveSupport.deprectator or our own deprecator. since https://github.com/rails/rails/pull/47354/files
* test: failing request posting sti with polymorphic has one * fix: polymorphic resource assignment * Add polymorphic_type_for method * Favor classify over singularize.camelize --------- Co-authored-by: lgebhardt <[email protected]>
* chore: fix file typo * fix: railtie to use correct load hook
* fix: more flexible polymorphic types lookup * test: add polymorphic lookup tests they pass on v-11-dev I'm going to look into the existing lookup warnings now ``` [POLYMORPHIC TYPE NOT FOUND] No polymorphic types found for fileable [POLYMORPHIC TYPE] No polymorphic types found for FilePropertiesResource fileable [POLYMORPHIC TYPE NOT FOUND] No polymorphic types found for respondent [POLYMORPHIC TYPE] No polymorphic types found for QuestionResource respondent [POLYMORPHIC TYPE NOT FOUND] No polymorphic types found for respondent [POLYMORPHIC TYPE] No polymorphic types found for AnswerResource respondent [POLYMORPHIC TYPE NOT FOUND] No polymorphic types found for keepable [POLYMORPHIC TYPE] No polymorphic types found for KeeperResource keepable ``` * Revert "test: add polymorphic lookup tests" This reverts commit 0979a7243b6bc816dd2327d3ff23f70209c52dce. * feat: easily clear the lookup * feat: add a descendents strategy * test: polymorphic type lookup * feat: make polymorphic type lookup configurable * feat: clear polymorphic lookup after initialize
* Add warning about disabling eager loading * Fix overriding polymorphic types on a relationship
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTE: This is a draft PR and it is expected that things will change. Currently this PR has been released as v0.11.0.beta2 to ease testing. I do not recommend using the beta release in a production environment.
In order to avoid maintaining the older branches and versions v0.11 will support retrieving resources in similar ways to v0.09 and v0.10 in addition to the changes made to the new default.
This brings together support for retrieving resources using the methods of the past versions. This can be controlled by the new configuration option
default_resource_retrieval_strategy. The following options are supported:JSONAPI::ActiveRelationRetrievalV09:JSONAPI::ActiveRelationRetrievalV10: Similar to how v0.10.x retrieves related resources by joining the related resource's model to the primary resource's model and merging in the related resource'srecords. Differences from the earlier version is the additional pluck queries have been eliminated in the case where caching is not used.JSONAPI::ActiveRelationRetrieval: This retrieves related resources directly and joins to the primary resource through the inverse relationship:noneand:self: The resource class, or a base class, is expected to implement or include the methods for retrieving resources.Additional Features to implement
records_for_populate. Instead definerecords_for_populateto call new methods for:serialization_includesfor includes needed for serialization.apply_serialization(name needs rework)