Update seed dependencies - #33
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
joaquimvila
left a comment
joaquimvila
left a comment
There was a problem hiding this comment.
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.
| cmake_minimum_required(VERSION 3.2) | ||
|
|
||
| # Find external dependencies | ||
| find_package(Boost) |
There was a problem hiding this comment.
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.
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.
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.
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
| { | ||
| long expirationSeconds = 600; // 10 minutes | ||
| auto& epochTimeService = m_context.getServicesMgr()->getSystemServicesMgr().getEpochTimeService(); | ||
| systelab::time::TimeAdapter timeAdapter; |
There was a problem hiding this comment.
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).
| 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.
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.
3 participants
No description provided.