-
Notifications
You must be signed in to change notification settings - Fork 44
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
feat: bind lookup service to Groovy constant for use in scripts #1166
feat: bind lookup service to Groovy constant for use in scripts #1166
Conversation
@@ -305,6 +306,9 @@ public static Binding createBinding(InstanceBuilder builder, Cell cell, Cell typ | |||
binding.setVariable(BINDING_INSTANCE_INDEX, | |||
executionContext.getService(InstanceIndexService.class)); | |||
|
|||
binding.setVariable(BINDING_LOOKUP_SERVICE, | |||
executionContext.getService(LookupService.class)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think a specific binding is preferable over the more generic approach to add a binding for the ServiceProvider
? (see ING-4256)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, if we can find a solution that avoids having to add individual bindings for every service class that we want to use in Groovy scripts, that would be better IMO. Happy to close this PR in favour of a more generic approach!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be alright if we squash the last two commits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be alright if we squash the last two commits?
@emanuelaepure10 Yes, no need to keep my initial commit.
99b7d04
to
748937d
Compare
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
8bc066b
to
f1e57c3
Compare
@@ -305,6 +306,9 @@ public static Binding createBinding(InstanceBuilder builder, Cell cell, Cell typ | |||
binding.setVariable(BINDING_INSTANCE_INDEX, | |||
executionContext.getService(InstanceIndexService.class)); | |||
|
|||
binding.setVariable(BINDING_SERVICE_PROVIDER, | |||
executionContext.getService(ServiceProvider.class)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emanuelaepure10 I think it must be
binding.setVariable(BINDING_SERVICE_PROVIDER, executionContext);
instead as ExecutionContext
implements the ServiceProvider
interface. That way, one can do e.g. the following in Groovy scripts to get an instance of LookupService
:
def lookupService = _serviceProvider.getService(LookupService.class)
f1e57c3
to
c9b7f98
Compare
c9b7f98
to
03510de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think to be usable this has to go in line with adding exceptions to the Groovy restrictions. At least for being able to call getService
on the service provider.
03510de
to
5866158
Compare
@@ -305,6 +306,13 @@ public static Binding createBinding(InstanceBuilder builder, Cell cell, Cell typ | |||
binding.setVariable(BINDING_INSTANCE_INDEX, | |||
executionContext.getService(InstanceIndexService.class)); | |||
|
|||
try { | |||
binding.setVariable(BINDING_SERVICE_PROVIDER, executionContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stempler I'm not very sure if by "adding exceptions to the Groovy restrictions." is this what you meant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emanuelaepure10 Simon refers to the Groovy restrictions that prevent using classes and methods in Groovy scripts that are not explicitly whitelisted. Check the class RestrictiveGroovyInterceptor.
This is an example for a PR where a class was whitelisted. As Simon suggested, in this case it might make sense to investigate if only calling getService(...)
should be allowed in contrast to whitelisting access to all methods of the ServiceProvider
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add to that likely the check is done against the concrete implementation and not the interface (ServiceProvider
) where the method is defined.
One solution for that could be creating a delegate class that implements the interface, then the delegate can be whitelisted and passed in here, regardless of what the actual implementation class of the execution context is here and what other methods it provides.
5866158
to
85dfae5
Compare
@@ -9,6 +9,7 @@ Require-Bundle: groovy;bundle-version="2.5.19", | |||
Export-Package: eu.esdihumboldt.util.groovy.sandbox, | |||
eu.esdihumboldt.util.groovy.sandbox.internal;x-internal:=true | |||
Import-Package: de.fhg.igd.eclipse.util.extension, | |||
eu.esdihumboldt.hale.common.core.service, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding a dependency here, the allowed class should be added via the extension point in the bundle where the DelegateServiceProvider
class is defined.
See here for an example.
0b84ee3
to
5e304ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I could imagine that for the use case with the lookup tables it could be necessary to add additional classes to be excluded from the Groovy restrictions, e.g the LookupServiceImpl.
Did you test if usage of the service is possible with active Groovy restrictions?
I have test it with and without |
Do you have any example of how to exclude a class from the Groovy restrictions? |
What you did for the |
But... the |
5e304ab
to
25a06d1
Compare
By adding it to the allowed classes, you excluded it from the restrictions when executing Groovy. It can be accessed without lifting the restrictions. |
Add an additional binding for the Service Provider to Groovy scripts. ING-4265
25a06d1
to
b6e43d1
Compare
🎉 This PR is included in version 5.2.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
ING-4256