Add support for field names in idenitity constraints #478
Conversation
This file contains hidden or 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
lcmen
commented
Feb 6, 2025
lib/data_layer.ex
Outdated
| ] | ||
| ) | ||
| identity = Enum.find(identities, fn identity -> | ||
| String.contains?(constraint, to_string(identity.name)) |
Contributor
Author
There was a problem hiding this comment.
It's probably not the right approach to match violated constraint with an identity but it was the quickest way to test this feature in my local env.
Contributor
There was a problem hiding this comment.
We have some logic elsewhere to get the name for a given identity, so we should reverse engineer it here. I believe we do this in the migration generator and elsewhere when we actually add in the constraint. So we can search the identities for a match on that name instead of just containing the identity name 😄
lcmen
force-pushed
the
add-support-for-field-names
branch
from
ab8b966 to
7486066
Compare
Contributor
Author
|
@zachdaniel please let me know if this is what you meant by:
In your original response. Thanks! |
Contributor
|
Hey there! Haven't forgotten about you, will review soon, hopefully today. |
Contributor
|
Looks great! As you pointed out, the main issue is how we get the relevant identity. It should be doable, search in the |
lcmen
force-pushed
the
add-support-for-field-names
branch
from
7486066 to
f137692
Compare
Contributor
Author
Thank you! I followed your suggestions and I've updated the logic to match identity to constraint. I would like to work on tests now but I have problems running tests for the but I have many failures across multiple files, as an example here is the output from Are those failures related to my setup? Thanks! |
lcmen
commented
Feb 9, 2025
|
|
||
| "main" -> | ||
| [git: "https://github.com/ash-project/ash.git"] | ||
| [git: "https://github.com/ash-project/ash.git", override: true] |
Contributor
Author
There was a problem hiding this comment.
If I don't add override, then I'm not able to use main option for ASH_VERSION variable.
Contributor
There was a problem hiding this comment.
Interesting, I haven't had that problem but adding that should fine.
Contributor
Author
There was a problem hiding this comment.
Without override: true, I was getting a following error:
Dependencies have diverged:
* ash (https://github.com/ash-project/ash.git)
the dependency ash in mix.exs is overriding a child dependency:
> In mix.exs:
{:ash, [env: :prod, git: "https://github.com/ash-project/ash.git"]}
> In deps/ash_sql/mix.exs:
{:ash, "~> 3.0 and >= 3.4.60", [env: :prod, hex: "ash", repo: "hexpm"]}
Ensure they match or specify one of the above in your deps and set "override: true"
** (Mix) Can't continue due to errors on dependencies
lcmen
commented
Feb 9, 2025
test/support/resources/post.ex
Outdated
| identities do | ||
| identity(:uniq_one_and_two, [:uniq_one, :uniq_two]) | ||
| identity(:uniq_on_upper, [:upper_thing]) | ||
| identity(:uniq_on_upper_title, [:upper_title], field_names: [:title]) |
Contributor
Author
There was a problem hiding this comment.
I would like to use this identity to test field_names option.
Contributor
Author
There was a problem hiding this comment.
I decided to use Organization resource as it's not heavily used in other tests. When I added a new constraint to the Post resource, some other tests started to fail.
Contributor
|
Don't worry about those tests 😄 They are related to other things that have been fixed elsewhere. |
6 tasks
Contributor
Author
|
It depends on ash-project/ash#1786 |
lcmen
force-pushed
the
add-support-for-field-names
branch
from
a16e7d5 to
0344284
Compare
Contributor
Author
|
Dependent PR has been merged. @zachdaniel can you give it a final review, please? |
Contributor
|
Looks great, however |
Contributor
Author
Thank you! Just to make sure I understand. Now I need to wait for new |
Contributor
|
Yeah we just need to pin to at least 3.4.64 |
Contributor
Author
Done. I believe now they can be released together. |
Contributor
|
🚀 Thank you for your contribution! 🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This PR kicks start the work I suggested in proposal to add field names to identities.
Please keep in mind the feature is not complete yet and it also relies on adding
field_namesoption toAsh.Resource.Identitythat I plan to address after this one is completed.Once the overall direction is approved, I will work on tests to cover this new functionality.
Please let me know what you think!
PS. It's my first constribution to any Elixir open source project. I only use Elixir in my spare time so please forgive all my mistakes. I treat is as an opportunity to learn more about Elixir and Ash ecosystem.
Contributor checklist