Skip to content
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

Make IdpMetadataParser#get_idp_metadata public #691

Merged

Conversation

calebhearth
Copy link

I have a use-case for being able to cache the intermediate fetch of
metadata in case of temporary failures, so rather than:

parser.parse_remote(url)

I'd like to

begin
  metadata = parser.get_idp_metadata(url, true)
  do_my_caching(metadata)
  parser.parse(metadata)
rescue HttpError
  load_cache
end

There's a fair amount of logic in the get_idp_metadata method that I'd
rather not need to re-implement. Right now I have this implemented with
parser.send(:get_idp_metadata, url, true) which is obviously not great
if the internals of this class change in the future. Can we move this
method to the public API?

I have a use-case for being able to cache the intermediate fetch of
metadata in case of temporary failures, so rather than:

    parser.parse_remote(url)

I'd like to

    begin
      metadata = parser.get_idp_metadata(url, true)
      do_my_caching(metadata)
      parser.parse(metadata)
    rescue HttpError
      load_cache
    end

There's a fair amount of logic in the get_idp_metadata method that I'd
rather not need to re-implement. Right now I have this implemented with
`parser.send(:get_idp_metadata, url, true)` which is obviously not great
if the internals of this class change in the future. Can we move this
method to the public API?
@johnnyshields
Copy link
Collaborator

Seems reasonable. @pitbulk I vote to merge.

@pitbulk pitbulk merged commit e827926 into SAML-Toolkits:master Jul 8, 2024
19 of 30 checks passed
@calebhearth calebhearth deleted the make-get_idp_metadata-public branch July 9, 2024 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants