RESTCOMM-2064: Refactor ExtensionController and implement getEffectiveConfiguration #2923
Conversation
…roller with ExtensionContext. Implement getEffectiveConfiguration in impl. Add init to ExtensionController to initialize ServletContext.
| OrganizationsDao od = daoManager.getOrganizationsDao(); | ||
| ExtensionConfiguration extCfg = ecd.getAccountExtensionConfiguration(scopeSid, extensionSid); | ||
|
|
||
| Sid sid = new Sid(scopeSid); |
There was a problem hiding this comment.
can we save cost of multiple queries here by querying the exact dao which sid belong to
| */ | ||
| package org.restcomm.connect.extension.api; | ||
|
|
||
| public interface ExtensionContext { |
There was a problem hiding this comment.
the idea of having a Context for extension was already in my mind. Could we inject this through the actual extension points, instead of using servletContext?
…quest. Set extensionContext in ExtensionController
| private final String id; | ||
|
|
||
| public enum Type { | ||
| ACCOUNT, APPLICATION, ANNOUNCEMENT, CALL, CLIENT, CONFERENCE, GATEWAY, INVALID, NOTIFICATION, PHONE_NUMBER, RECORDING, REGISTRATION, SHORT_CODE, SMS_MESSAGE, TRANSCRIPTION, INSTANCE, EXTENSION_CONFIGURATION, GEOLOCATION, ORGANIZATION, PROFILE | ||
| CALL(null, CALL_SID_STRING), |
There was a problem hiding this comment.
not sure how this Sid refactoring is related to original PR issue, but im fine with it
There was a problem hiding this comment.
went overboard with it, we just needed to add a getType to the Sid because we dont know what the incoming Sid is in getEfectiveConfiguration
| @@ -43,6 +44,8 @@ | |||
| public ExtensionBootstrapper(final ServletContext context, final Configuration configuration) { | |||
| this.configuration = configuration; | |||
| this.context = context; | |||
| this.context.setAttribute(ExtensionContext.class.getName(), ExtensionController.getInstance()); | |||
There was a problem hiding this comment.
do we need this anylonger?
| /** | ||
| * @param set ExtensionContext | ||
| */ | ||
| void setExtensionContext(ExtensionContext ec); |
There was a problem hiding this comment.
not sure we need the setter in the interface, but is a minor thing now
abdulazizali77
force-pushed
the
RESTCOMM-2064
branch
from
1007ee0 to
e59b2bd
Compare
|
Team, I did a review in this branch and here is my comment related to the Configuration sets we make available to the Extensions. So we have two sets of
The separation of concerns between those two configuration sets is clear but the naming used so far is ambiguous and doesn't make clear what is the role of each configuration sets. For this reason, I suggest a name change for the In future work on the Extension API, where we plan for handling configuration the same way we handle Profiles, we should merge the two sets of configuration (DefaultConfiguration and ExtensionRules) and let the ExtensionController provide the methods to retrieve each subset of the conf, for example
As part of this story and PR, I will provide the Important |
This close #RESTCOMM-2099
gvagenas
force-pushed
the
RESTCOMM-2064
branch
from
f4a87fe to
910ff61
Compare
gvagenas
force-pushed
the
master
branch
2 times, most recently
from
9469191 to
c67f142
Compare
gvagenas
force-pushed
the
RESTCOMM-2064
branch
from
910ff61 to
b58377d
Compare
What this PR does / why we need it:
Centralize configuration hierarchy lookup in
getEffectiveConfiguration. Add anExtensionContextinterface for implementing extensions to access public extension framework functionality.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes # RESTCOMM-2064, RESTCOMM-1617
Special notes for your reviewer:
Decided to go with
ExtensionContextinstead ofExtensionControllerFacade.One other possible option is to not have
ExtensionControllerimplementExtensionContextbut create newExtensionContextImplclass instead