-
-
Notifications
You must be signed in to change notification settings - Fork 364
fix: manual relationship implies no_attributes? true #2562
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
fix: manual relationship implies no_attributes? true #2562
Conversation
Adds a transformer on relationships.{has_one, has_many} DSLs to automatically set `no_attributes?` to `true` if `manual` is set.
Reworks logic in places that use `no_attributes?` and `manual` together to take this new implication into consideration. (read.ex, relationships.ex, and schema.ex)
Adds some tests to ensure that this implication holds. Also adds regression tests to ensure that `select` callback for manual relationships is respected.
| # | ||
| # SPDX-License-Identifier: MIT | ||
|
|
||
| defmodule Ash.Resource.Relationships.SharedTransformers do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a different shared module that we use for shared helpers across relationships, lets put it there instead of adding a new module: Ash.Resource.Relationships.SharedOptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in latest commit.
Also opened ash-project/ash_sql#213 to fix the issue with this PR + ash_postgres.
|
ash_postgres may need some changes too, looking into it |
|
Yeah it seems like ash_postgres has some failures in certain tests with ash set to this branch. Will take a look tmmrw! |
…ggregates 1. Refactor a large conditional branch in `AshSql.Aggregate.get_subquery` to use a new multi-head private function instead of nested ifs. 2. Make it so that the case where the relationship is manual matches BEFORE the case where the relationship is no_attributes?: true This is necessary in order to pass ash_postgres tests when running against ash with this PR applied: ash-project/ash#2562
|
Basically, like the issue in ash_sql, any ash extensions / data layers or whatever that have code that tests something like: Has the potential to break since now after this PR the no_attributes? branch will execute instead of the manual one |
|
^ So they can pretty much be fixed by just doing the manual case first. (like I did in the ash_sql PR) |
…ggregates (#213) 1. Refactor a large conditional branch in `AshSql.Aggregate.get_subquery` to use a new multi-head private function instead of nested ifs. 2. Make it so that the case where the relationship is manual matches BEFORE the case where the relationship is no_attributes?: true This is necessary in order to pass ash_postgres tests when running against ash with this PR applied: ash-project/ash#2562
|
🚀 Thank you for your contribution! 🚀 |
Adds a transformer on relationships.{has_one, has_many} DSLs to automatically set
no_attributes?totrueifmanualis set.Reworks logic in places that use
no_attributes?andmanualtogether to take this new implication into consideration. (read.ex, relationships.ex, and schema.ex)Adds some tests to ensure that this implication holds. Also adds regression tests to ensure that
selectcallback for manual relationships is respected.Notes
no_attributes?: falsein manual relationships? (That is, change the transformer logic to NOT setno_attributes?: trueif they've manually specified it asfalsein the DSL)Contributor checklist
Leave anything that you believe does not apply unchecked.