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

Filtering function in MetaLoader #288

Closed

Conversation

pradtke
Copy link
Contributor

@pradtke pradtke commented Nov 17, 2015

Testable with:

phpunit --configuration tools/phpunit tests/modules/metarefresh/lib/MetaLoaderTest.php

Change is add ability to filter entities by a callback function (or a currying function). The main use case is filtering metadata that has eduGain entities by the registration authority.

'source' can now take new keys:

'filterCallback' is the name of a function that accepts a 'SimpleSAML_Metadata_SAMLParser' and returns true/false

'filterFactory' is the name of a function that returns a function that meets the requirements of 'filterCallback'. 'filterFactoryArgs' is an array of arguments passed to the factory function.


One thing I could not figure out was how to enable the metarefresh module for unit test. Jamie described a technique in https://groups.google.com/d/msg/simplesamlphp-dev/Sp0Hhe86KTQ/pzY6LubrAAAJ however (because of lack of php and simplesaml experience) I couldn't get to work. For short term I added an ENV variable to load all modules and that ENV gets set by phpunit. I will need some help on this part if we want to use a different technique.

If we have some agreement on the new attribute names and the functionality then I can work on updating the docs in a subsequent PR

Lastly, my co-worker pointed out that the filtering is happening post XML processing/conversion to entities which is not memory efficient. It might be desirable to move filtering into the SimpleSaml SimpleSAML_Metadata_SAMLParser::parseDescriptorsElement() call or to have that call return a generator so that only one entity is converted/filtered at a time.

CONTRIBUTE.md Outdated
@@ -64,6 +64,7 @@ deploy, and therefore we try to avoid it.
* Add **unit tests** to verify that your code not only works but also keeps working over time. When adding tests, keep
the same directory structure used for regular classes. Try to cover **all your code** with tests. The bigger the test
coverage, the more reliable and better our library is.
* `phpunit --configuration tools/phpunit tests/path/to/your/test.php`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to elaborate a bit more.

On the other hand, as long as the test is inside the tests directory, there's no need to pass it in the command line, as phpunit will run all tests it finds in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added more info. My goal was for someone not well versed in php to be able to create/run tests.

@jaimeperez
Copy link
Member

Thanks a lot for this PR @pradtke! Outstanding job!

I've commented on a few things in the code itself. I think the main things we need to address in order to merge this PR are resolving the conflicts, and splitting the PR into 2 (maybe more), so that things are self-contained. Other than that, it's a great work 😄

@pradtke
Copy link
Contributor Author

pradtke commented Feb 24, 2016

Thanks @jaimeperez I've run through the fixes. I broke out 1 PR (and can remove that code from this, once its approved). As mentioned in the comments I'm unable to get an alternate way to enable a module for testing to work. I agree my way is wrong :) It was mostly just to get the tests to run.

@jtgasper3
Copy link
Contributor

Hey all,

I'm looking for this functionality to filter out entities so that my SSP instance only has InCommon Research and Scholarship tagged IdPs. Is the only outstanding issue the merge conflicts?

Thanks,
John

@pradtke
Copy link
Contributor Author

pradtke commented Apr 18, 2017

I still need to get the tests working in a better fashion as a prerequisite for merging. I'll go ahead and fix the PR to avoid conflicts.

@jtgasper3
Copy link
Contributor

jtgasper3 commented Apr 18, 2017 via email

@pradtke pradtke force-pushed the feature/callback-metadata-filter branch from 6d09f7b to 38dae4c Compare April 18, 2017 18:37
@pradtke
Copy link
Contributor Author

pradtke commented Apr 18, 2017

@jtgasper3 I've done an update. So it will apply cleanly to master, and should apply to 1.14.x branches if you limit the patch files to modules/metarefresh/lib/CommonFilters.php and modules/metarefresh/lib/MetaLoader.php - all the other files are test or documentation related.

I think the major change between 1.14 and 1.13 is that I had pulled out the RegistrationAuthority changes into their own patch that is part of 1.14.

To preserve 1.13 compatibility I created a copy of the branch at https://github.com/pradtke/simplesamlphp/tree/feature/callback-metadata-filter-1_13 and there is also a gist https://gist.github.com/pradtke/94422f5534c69a02bae7.

In regards to tests and getting this PR approved, I started this mailing list discussion https://groups.google.com/forum/#!topic/simplesamlphp-dev/OKfZtXrc3P0 to figure out how the project wants to handle global state spill over between tests.

@tvdijen
Copy link
Member

tvdijen commented May 28, 2018

@pradtke I think the recently merged #819 will solve your unit testing issues

@tvdijen tvdijen force-pushed the master branch 3 times, most recently from 1c686ab to eb20457 Compare August 17, 2020 20:43
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from 08ebb9c to 64fca25 Compare July 2, 2021 14:12
@tvdijen
Copy link
Member

tvdijen commented Sep 8, 2021

@pradtke This is so outdated and the module has been moved to it's own repository in the meantime.
If this is still desirable functionality, would you mind creating a new PR in the metarefresh-repo?

@tvdijen tvdijen closed this Sep 8, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants