dimas-b commented on code in PR #493: URL: https://github.com/apache/polaris/pull/493#discussion_r1867083573
########## polaris-service/src/main/resources/META-INF/hk2-locator/default: ########## @@ -0,0 +1,88 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +[org.apache.polaris.service.auth.DefaultPolarisAuthenticator]S +contract={org.apache.polaris.service.auth.BasePolarisAuthenticator} +name=default +qualifier={jakarta.inject.Named} + +[org.apache.polaris.service.auth.TestInlineBearerTokenPolarisAuthenticator]S +contract={io.dropwizard.auth.Authenticator} +name=test +qualifier={jakarta.inject.Named} Review Comment: I'm not sure I understand the purpose of this declaration. It seems rather redundant to me. The class of `TestInlineBearerTokenPolarisAuthenticator` already has the `@Named("test")` annotation and implements `Authenticator`. This file does not appear to add any new information to what is already known from class files :thinking: ########## polaris-service/src/main/java/org/apache/polaris/service/config/PolarisApplicationConfig.java: ########## @@ -63,69 +88,206 @@ public class PolarisApplicationConfig extends Configuration { private String awsSecretKey; private FileIOFactory fileIOFactory; private RateLimiter rateLimiter; + private TokenBrokerFactory tokenBrokerFactory; private AccessToken gcpAccessToken; public static final long REQUEST_BODY_BYTES_NO_LIMIT = -1; private long maxRequestBodyBytes = REQUEST_BODY_BYTES_NO_LIMIT; + @Inject ServiceLocator serviceLocator; + + @PostConstruct + public void bindToServiceLocator() { + ServiceLocatorUtilities.bind(serviceLocator, binder()); + } + + @NotNull + public AbstractBinder binder() { + PolarisApplicationConfig config = this; + return new AbstractBinder() { + @Override + protected void configure() { + bindFactory(SupplierFactory.create(serviceLocator, config::getStorageIntegrationProvider)) + .to(PolarisStorageIntegrationProvider.class) + .ranked(OVERRIDE_BINDING_RANK); + bindFactory(SupplierFactory.create(serviceLocator, config::getMetaStoreManagerFactory)) + .to(MetaStoreManagerFactory.class) + .ranked(OVERRIDE_BINDING_RANK); + bindFactory(SupplierFactory.create(serviceLocator, config::createConfigurationStore)) + .to(PolarisConfigurationStore.class) + .ranked(OVERRIDE_BINDING_RANK); + bindFactory(SupplierFactory.create(serviceLocator, config::getFileIOFactory)) + .to(FileIOFactory.class) + .ranked(OVERRIDE_BINDING_RANK); + bindFactory(SupplierFactory.create(serviceLocator, config::getPolarisAuthenticator)) + .to(Authenticator.class) + .ranked(OVERRIDE_BINDING_RANK); + bindFactory(SupplierFactory.create(serviceLocator, config::getTokenBrokerFactory)) + .to(TokenBrokerFactory.class) + .ranked(OVERRIDE_BINDING_RANK); + bindFactory(SupplierFactory.create(serviceLocator, config::getOauth2Service)) + .to(IcebergRestOAuth2ApiService.class) + .ranked(OVERRIDE_BINDING_RANK); + bindFactory(SupplierFactory.create(serviceLocator, config::getCallContextResolver)) + .to(CallContextResolver.class) + .ranked(OVERRIDE_BINDING_RANK); + bindFactory(SupplierFactory.create(serviceLocator, config::getRealmContextResolver)) + .to(RealmContextResolver.class) + .ranked(OVERRIDE_BINDING_RANK); + bindFactory(SupplierFactory.create(serviceLocator, config::getRateLimiter)) + .to(RateLimiter.class) + .ranked(OVERRIDE_BINDING_RANK); + } + }; + } + + /** + * Factory implementation that uses the provided supplier method to retrieve the instance and then + * uses the {@link #serviceLocator} to inject dependencies into the instance. This is necessary + * since the DI framework doesn't automatically inject dependencies into the instances created. + * + * @param <T> + */ + private static final class SupplierFactory<T> implements Factory<T> { + private final ServiceLocator serviceLocator; + private final Supplier<T> supplier; + + private static <T> SupplierFactory<T> create( + ServiceLocator serviceLocator, Supplier<T> supplier) { + return new SupplierFactory<>(serviceLocator, supplier); + } + + private SupplierFactory(ServiceLocator serviceLocator, Supplier<T> supplier) { + this.serviceLocator = serviceLocator; + this.supplier = supplier; + } + + @Override + public T provide() { + T obj = supplier.get(); + serviceLocator.inject(obj); + return obj; + } + + @Override + public void dispose(T instance) {} + } + + public <T> T findService(Class<T> serviceClass) { Review Comment: This seems to be a CDI concern rather than a configuration concern. I mean that it is perfectly fine to drive CDI with config settings, but `findService` does not appear to fit the DI paradigm :thinking: -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
