Skip to content

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

PacoGutierrez
Copy link

No description provided.

PacoGutierrez and others added 6 commits June 28, 2021 10:14
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.
default options updated to boost: 1.72.0, openssl: 1.1.1k, gtest: 1.10.0
Copy link
Collaborator

@joaquimvila joaquimvila left a 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)
Copy link
Collaborator

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.

Copy link
Author

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")
Copy link
Collaborator

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).

Copy link
Author

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;
Copy link
Collaborator

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;
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants