feat: set tenant from record in case of attribute multitenancy #195
Conversation
|
Makes sense to me, but we would need to do this: https://hexdocs.pm/ash/dsl-ash-resource.html#multitenancy-parse_attribute and parse the attribute. I'm not 100% sure on if it should be opt-in or not. We currently set the tenant only from the query which is done via list_tenants (which is TBH itself unideal how we do that). This change is theoretically breaking existing usage of resources that may be used that support bypassing multi tenancy? |
The changing behaviour is why I suggested maybe adding a flag. but I'm not sure it was actually possible to bypass multitenancy like this, because the |
Is that doing anything? The attribute of the record should already have the parsed value? |
|
If the tenant is something like |
I can PR that if you are open to it. I do think this would be pretty useful in ash oban |
|
Open to it, yes 👍 Must be opt in for this case, but otherwise sounds great. |
This would allow using a multitenancy `:allow_global` read action for the scheduler and a regular read for the worker_read_action, by setting the tenant from the attribute.
Add a DSL option `use_tenant_from_record?` (default: false) to gate the tenant extraction feature from records. When set to true, the tenant will be extracted from the record's tenant attribute when building trigger jobs. This allows using a multitenancy :allow_global read action for the scheduler and a regular read action for the worker_read_action. Changes: - Add use_tenant_from_record? to Trigger schema with documentation - Update Trigger typespec and defstruct - Gate tenant extraction logic in build_trigger function - Register new verifier in extension configuration When false (default), only explicit :tenant options are used, maintaining backward compatibility.
Add a verifier that ensures when use_tenant_from_record? is enabled, the multitenancy parse_attribute and tenant_from_attribute options are consistently configured - either both at defaults or both customized. These options are inverses of each other, so if one is customized, the other must also be customized to maintain correct tenant handling. The verifier provides clear error messages explaining the requirement and showing the current configuration state.
Update the test trigger to use the new use_tenant_from_record? flag, ensuring the tenant extraction feature is explicitly enabled for testing.
Add use_tenant_from_record? to locals_without_parens in formatter configuration to ensure consistent DSL formatting without parentheses. Update test to use the correct format: `use_tenant_from_record? true` instead of `use_tenant_from_record?(true)`.
Add use_tenant_from_record? option at the oban DSL level, allowing it to be set as a default for all triggers in a resource. The trigger-level option overrides the oban-level setting, following the same pattern as shared_context?. Changes: - Add use_tenant_from_record? to oban schema with documentation - Update SetDefaults transformer to inherit oban-level setting - Maintain backward compatibility with default: false
barnabasJ
force-pushed
the
feat/set-tenant-from-record
branch
from
1092e46 to
a48ce8d
Compare
Update documentation to be more user-focused, avoiding internal implementation details like build_trigger. Changes: - Trigger-level: Focus on scheduler using :allow_global while worker gets correct tenant context per record - Oban-level: Clarify it's a default value that can be overridden - Remove references to internal build_trigger function - Emphasize the user-visible behavior and use case
test/tenant_extraction_test.exs
Outdated
| Oban.drain_queue(queue: :default) | ||
| Oban.drain_queue(queue: :custom) |
There was a problem hiding this comment.
Is the goal to clear the queues out before tests, or to execute whichever jobs are left in there? If the goal is to clear them out I would reach for Repo.delete_all(Job, prefix: "private") instead.
There was a problem hiding this comment.
That makes more sense. Thanks for the tip.
Point ash dependency to GitHub main branch to access the new multitenancy_tenant_from_attribute API function that is not yet available in the released version.
Spark verifier errors are emitted as compile-time warnings to stderr, not as Logger messages. This caused tests using ExUnit.CaptureLog to fail because the error output was never captured. Changes: - Import ExUnit.CaptureIO instead of using CaptureLog - Use capture_io(:stderr, fn -> ... end) to capture compiler warnings - Improve assertions with regex patterns to match specific error format - Use ~r/parse_attribute:.*\(customized\)/ to verify attribute state - Assert output == "" for success cases (no errors emitted) The regex patterns verify that both the attribute name and its state (customized/default) appear on the same line, providing more precise validation than loose string matching.
This would allow using a multitenancy
:allow_globalread actionfor the scheduler and a regular read for the worker_read_action,
by setting the tenant from the attribute.
I could also add a flag for it if we want to keep it optional?
Contributor checklist
Leave anything that you believe does not apply unchecked.