-
Notifications
You must be signed in to change notification settings - Fork 2
Update seed dependencies #33
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
base: master
Are you sure you want to change the base?
Conversation
Modified default options Added build requeriments
Included TimeAdapter header to fix incompatibilities in both RouteAccessValidatorsFactory and SystemServicesFactory class
Also added the boost package as a dependency for SeedCppCore project.
…the visual studio files
default options updated to boost: 1.72.0, openssl: 1.1.1k, gtest: 1.10.0
…b/seed-cpp into update-seed-dependencies
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.
Thanks for the contribution! However, there are some comments that need to be addressed before integrating this PR. In case you need further details, please contact with me.
@@ -1,12 +1,18 @@ | |||
cmake_minimum_required(VERSION 3.2) | |||
|
|||
# Find external dependencies | |||
find_package(Boost) |
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.
New packages shouldn't be required because they are transitive dependencies of existing packages. I think previous version of this file should be kept.
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 agree not to add the new packages, but the conversion from boost to Boost is necessary
self.build_requires("JSONAdapterTestUtilities/1.1.3@systelab/stable") | ||
self.build_requires("JWTUtilsTestUtilities/1.1.7@systelab/stable") | ||
self.build_requires("JSONSettingsTestUtilities/1.3.8@systelab/stable") | ||
self.build_requires("gtest/1.10.0") |
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.
Version of google test package should be gathered from the options considering that now there are multiple values (1.10.0 and 1.8.1).
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.
In the options, it is contemplated to use version 1.8.1 or 1.10.0. Version 1.10.0 is required since the rest of the dependencies use it
@@ -23,9 +25,9 @@ namespace seed_cpp { namespace rest { | |||
std::unique_ptr<IRouteAccessValidator> RouteAccessValidatorsFactory::buildTokenExpirationRouteAccessValidator() const | |||
{ | |||
long expirationSeconds = 600; // 10 minutes | |||
auto& epochTimeService = m_context.getServicesMgr()->getSystemServicesMgr().getEpochTimeService(); | |||
systelab::time::TimeAdapter timeAdapter; |
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.
The TimeAdapter shouldn't be declared as a local variable because it is injected to the service being built, but it will be destroyed once exiting the factory method. Thus, this will cause a crash when accessing to an object that has been destroyed from the service.
Thus, I would suggest defining the TimeAdapter as part of the context, as we do with other adapters (i.e db adapter). Moreover, it should be injected on constructor of the Context object and as an interface (ITimeAdapter), so we allow using a mock version of it (we are not assuming to have always the real implementation).
@@ -31,7 +33,9 @@ namespace seed_cpp { namespace service { | |||
|
|||
std::unique_ptr<systelab::rest_api_core::IEpochTimeService> SystemServicesFactory::buildEpochTimeService() const | |||
{ | |||
return std::make_unique<systelab::rest_api_core::EpochTimeService>(); | |||
systelab::time::TimeAdapter TimeAdapter; |
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.
After the change on REST API core to not require an epoch service to build a TokenExpirationAccessValidator, I think that this method is not used anywhere else. If that's the case, I would get rid of this factory method.
No description provided.