♻️ Refactor / Rewrite stale_time/1 in idiomatic Elixir (#14)#28
Merged
Conversation
Drop the verbatim-from-esaml structure that relied on variable shadowing inside nested `case` blocks and triggered two Dialyzer pattern_match suppressions. The new form collects candidate expiries from the Subject and Conditions, takes `Enum.min/1`, and falls back to `issue_instant + 5 minutes` when neither restriction is present — same contract, no shadowing, no `# credo:disable` directive needed. Also adds dedicated unit tests for `stale_time/1` covering all three branches and the min selection in both directions, and removes the two `saml.ex:803` entries from `.dialyzer_ignore.exs` (the `sp_handler.ex:95` defensive entry stays — out of scope for this refactor).
Tighten `conditions_expiry/1` from a two-arm `case` into the equivalent `if stamp = Keyword.get(...), do: to_secs(stamp)` — same nil-or-value behavior, one line.
Actalab
approved these changes
May 19, 2026
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
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.
Summary
Closes #14 (scoped to
stale_time/1; broader esaml-port audit deferred — see below).Rewrites
ExSaml.Core.Saml.stale_time/1to drop the verbatim-from-arekinath/esamlstructure that relied on variable shadowing inside nestedcaseblocks. That shadowing made an inner branch tautological for Dialyzer and forced twopattern_matchsuppressions in.dialyzer_ignore.exs.The new form:
subject.notonorafter,conditions[:not_on_or_after]) into a list, rejectsnil, takesEnum.min/1.issue_instant + 5 * 60when neither restriction is present.@spec stale_time(Assertion.t()) :: integer()— same behavior, no shadowing.# credo:disable-for-next-line Credo.Check.Refactor.CyclomaticComplexitydirective (no longer needed).lib/ex_saml/core/saml.ex:803entries from.dialyzer_ignore.exs. Thesp_handler.ex:95defensive entry stays (out of scope, deliberate catch-all).Tests
Adds a new
describe "stale_time/1"block intest/ex_saml/core/saml_test.exswith five cases:issue_instant + 5 minwhen neither subject nor conditions carryNotOnOrAfter.These tests were written before the refactor and validated against the old implementation first, confirming behavioral parity.
Scope note
Issue #14 also asks to audit other esaml-ported functions for similar patterns. The audit found that
decode_response/1,decode_assertion/1,decode_logout_request/1,to_xml/1etc. already use idiomatic Elixir (with,Enum.reduce, functional accumulators).stale_time/1was the only function forcing Dialyzer suppressions due to esaml-style code. A follow-up issue can track any additional cosmetic cleanup if desired.Test plan
mix test— 204 tests, 0 failuresmix credo --strict lib/ex_saml/core/saml.ex— no issuesmix dialyzer— passed, 0 unnecessary skips (confirms thesp_handler.ex:95entry is still needed and the removedsaml.ex:803entries were the only ones tied to this refactor)