cBioPortal / cbioportal

cBioPortal for Cancer Genomics
https://cbioportal.org
GNU Affero General Public License v3.0
649 stars 506 forks source link

old issue back? #5816

Closed pieterlukasse closed 5 years ago

pieterlukasse commented 5 years ago

@n1zea144 we have a deployment where the exception mentioned here https://github.com/cBioPortal/cbioportal/pull/4611 : Caused by: org.apache.ibatis.builder.IncompleteElementException: Could not find SQL statement to include with refid 'org.cbioportal.persistence.mybatis.PatientMapper.select' shows up again when deploying v2.1.0. Any ideas what could be the cause?

n1zea144 commented 5 years ago

@pieterlukasse Not off the top of my head, but @khzhu just ran into this issue. Maybe she solved it?

khzhu commented 5 years ago

@pieterlukasse @n1zea144 , I got the same error when tried to deploy 2.1.0. You would not get this error if set -Dauthenticate=false or left out this option, but only when -Dauthenticate=saml. It might be related to the change made to authenticate flag in portal.properties file. when did this flag be deprecated?

khzhu commented 5 years ago

I rolled back to 2.0.1 to avoid the error.

caaespin commented 5 years ago

I'm having a similar issue on 2.1.0, I'm using saml so there's no way I can avoid it. I'm using the docker deployment described in the documentation, but there is no tag for version 2.0.1

jjgao commented 5 years ago

@khzhu would you please look into this as we are also using saml but don't have this issue here? Thanks.

khzhu commented 5 years ago

@jjgao , working on the issue, not sure if is keycloak specific issue. @caaespin, which identity provider were you using?

caaespin commented 5 years ago

@khzhu I'm using my institution's own custom built IdP. It's based on Shibboleth.

khzhu commented 5 years ago

I see, thanks @caaespin. I noticed -Dauthenticate=saml JVM option had no effect at all. was authenticate flag included in your portal.properties? if not can you please add

authenticate=saml

after line authorization=true see what happens?

caaespin commented 5 years ago

@khzhu I had those properties already set up. I just put moved authenticate=saml so it would be literally below authorization=true. After redeployment, I still get the same error. Here is the beginning of the stacktrace:

org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'studyViewFilterApplier' defined in URL [jar:file:/usr/local/tomcat/webapps/cbioportal/WEB-INF/lib/web-2.1.0-SNAPSHOT.jar!/org/cbioportal/web/util/StudyViewFilterApplier.class]: Unsatisfied dependency expressed through constructor parameter 0; nested exception is org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'sampleServiceImpl': Unsatisfied dependency expressed through field 'studyService'; nested exception is org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'studyServiceImpl': Unsatisfied dependency expressed through field 'cancerTypeService'; nested exception is org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'cancerTypeServiceImpl': Invocation of init method failed; nested exception is org.apache.ibatis.builder.IncompleteElementException: Could not find SQL statement to include with refid 'org.cbioportal.persistence.mybatis.PatientMapper.select'

and further down here is the other piece of the stack trace

Caused by: org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'sampleServiceImpl': Unsatisfied dependency expressed through field 'studyService'; nested exception is org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'studyServiceImpl': Unsatisfied dependency expressed through field 'cancerTypeService'; nested exception is org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'cancerTypeServiceImpl': Invocation of init method failed; nested exception is org.apache.ibatis.builder.IncompleteElementException: Could not find SQL statement to include with refid 'org.cbioportal.persistence.mybatis.PatientMapper.select'
khzhu commented 5 years ago

@caaespin , thanks for the testing! you were using docker right? did you include a JVM option for db connection pool?

CATALINA_OPTS='-Ddbconnector=dbcp'

caaespin commented 5 years ago

@khzhu yes indeed I am using docker. I also have that option enabled.

khzhu commented 5 years ago

@caaespin , will dig deep into the issue...thanks!

khzhu commented 5 years ago

@fedde-s, I took a look at those changed you made to JdbcUtil.java file, 2.1.0 initiates a new JdbcDataSource object, while old version was calling a private static method. Could that be a problem or the reason no connection established from dbcp? thanks!

fedde-s commented 5 years ago

@khzhu

Could [the introduction of the JdbcDataSource constructor] be a problem or the reason no connection established from dbcp?

I can't see how that would affect this, no; it seems related more to Batis and/or Spring than to the actual database connection.

pieterlukasse commented 5 years ago

@khzhu I would rather look at recent spring configuration changes that could be causing this (think of application*context.xml files).

pieterlukasse commented 5 years ago

@khzhu @n1zea144 my guess is that recent code was introduced that uses PatientRepository indirectly, and this new code is not adding PatientRepository as a workaround, like done here https://github.com/cBioPortal/cbioportal/blame/v2.1.0/security/security-spring/src/main/java/org/cbioportal/security/spring/CancerStudyPermissionEvaluator.java#L68. By the way: I hope we can find another workaround or a solution as the current workaround is obviously too error prone. Why would one have to include an autowired item to the code if it is not used anywhere? This seems wrong. Adding @ersinciftci to the discussion.

khzhu commented 5 years ago

thanks @pieterlukasse @fedde-s ! For my case, the root cause more likely was the dbcp authentication failure (please see logs below). There was not any changes made to the database before updating cBioPortal from 2.0.1 to 2.1.0. Thus, user 'cbio_user' should not be denied, right? Only thing I could think of was the DataSource object was not initialized with user 'cbio_user' credential. In PR #4787 you proposed to replace a private static initDataSource() method with a new DataSource object creation. This newly instantiated dataSource object was pushed to the stack rather than stored in a heap space to make the connection available for future requests to the database.

Caused by: org.mybatis.spring.MyBatisSystemException: nested exception is org.apache.ibatis.exceptions.PersistenceException: Caused by: org.apache.ibatis.exceptions.PersistenceException: Caused by: org.springframework.jdbc.CannotGetJdbcConnectionException: Could not get JDBC Connection; nested exception is java.sql.SQLException: Cannot create PoolableConnectionFactory (Access denied for user 'cbio_user'@'localhost' (using password: YES)) Caused by: java.sql.SQLException: Cannot create PoolableConnectionFactory (Access denied for user 'cbio_user'@'localhost' (using password: YES)) Caused by: java.sql.SQLException: Access denied for user 'cbio_user'@'localhost' (using password: YES) Caused by: org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'sampleServiceImpl': Unsatisfied dependency expressed through field 'studyService'; nested exception is org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'studyServiceImpl': Unsatisfied dependency expressed through field 'cancerTypeService'; nested exception is org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'cancerTypeServiceImpl': Invocation of init method failed; nested exception is org.apache.ibatis.builder.IncompleteElementException: Could not find SQL statement to include with refid 'org.cbioportal.persistence.mybatis.PatientMapper.select' Caused by: org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'studyServiceImpl': Unsatisfied dependency expressed through field 'cancerTypeService'; nested exception is org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'cancerTypeServiceImpl': Invocation of init method failed; nested exception is org.apache.ibatis.builder.IncompleteElementException: Could not find SQL statement to include with refid 'org.cbioportal.persistence.mybatis.PatientMapper.select' Caused by: org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'cancerTypeServiceImpl': Invocation of init method failed; nested exception is org.apache.ibatis.builder.IncompleteElementException: Could not find SQL statement to include with refid 'org.cbioportal.persistence.mybatis.PatientMapper.select' Caused by: org.apache.ibatis.builder.IncompleteElementException: Could not find SQL statement to include with refid 'org.cbioportal.persistence.mybatis.PatientMapper.select' Caused by: java.lang.IllegalArgumentException: XML fragments parsed from previous mappers does not contain value for org.cbioportal.persistence.mybatis.PatientMapper.select Caused by: org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'sampleServiceImpl': Unsatisfied dependency expressed through field 'studyService'; nested exception is org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'studyServiceImpl': Unsatisfied dependency expressed through field 'cancerTypeService'; nested exception is org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'cancerTypeServiceImpl': Invocation of init method failed; nested exception is org.apache.ibatis.builder.IncompleteElementException: Could not find SQL statement to include with refid 'org.cbioportal.persistence.mybatis.PatientMapper.select' Caused by: org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'studyServiceImpl': Unsatisfied dependency expressed through field 'cancerTypeService'; nested exception is org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'cancerTypeServiceImpl': Invocation of init method failed; nested exception is org.apache.ibatis.builder.IncompleteElementException: Could not find SQL statement to include with refid 'org.cbioportal.persistence.mybatis.PatientMapper.select' Caused by: org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'cancerTypeServiceImpl': Invocation of init method failed; nested exception is org.apache.ibatis.builder.IncompleteElementException: Could not find SQL statement to include with refid 'org.cbioportal.persistence.mybatis.PatientMapper.select' Caused by: org.apache.ibatis.builder.IncompleteElementException: Could not find SQL statement to include with refid 'org.cbioportal.persistence.mybatis.PatientMapper.select'

fedde-s commented 5 years ago

Do you have a file named portal.properties in the folder referenced by the $PORTAL_HOME variable of the webserver process? If so, the webserver should now consistently use that as the configuration file instead of defaulting to the one baked into the WAR file.

khzhu commented 5 years ago

@fedde-s , yes we do have portal.properties file under $PORTAL_HOME. As I mentioned, there was any changes made to the system prior to the upgrading. Plus, portal.properties file was not included in the WAR file, so should not be any issues.

pieterlukasse commented 5 years ago

@khzhu the error states:

Caused by: java.sql.SQLException: Access denied for user 'cbio_user'@'localhost' (using password: YES)

Is your DB running on localhost?

khzhu commented 5 years ago

@pieterlukasse , yes, on the same VM as cbioportal.

oplantalech commented 5 years ago

@khzhu that's weird, I've set up cBioPortal v2.1.0 with Keycloak and I don't have issues, just works fine. I'm using the Dauthenticate=saml option.

khzhu commented 5 years ago

@oplantalech , our production server is a centos7 VM, with tomcat9, java8, and mysql8 installed. We had no issues with 2.0.1 but 2.1.0.

khzhu commented 5 years ago

also, keycloak is not running from the same VM.

oplantalech commented 5 years ago

@khzhu I get the same error if the portal.properties file is not present at runtime.

12-Apr-2019 15:26:29.524 SEVERE [localhost-startStop-1] org.apache.catalina.core.StandardContext.listenerStart Exception sending context initialized event to listener instance of class [org.springframework.web.context.ContextLoaderListener] cbioportal-container | org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'clinicalDataEqualityFilterApplier' defined in URL [jar:file:/usr/local/tomcat/webapps/ROOT/WEB-INF/lib/web-2.1.0-SNAPSHOT.jar!/org/cbioportal/web/util/ClinicalDataEqualityFilterApplier.class]: Unsatisfied dependency expressed through constructor parameter 0; nested exception is org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'patientServiceImpl': Unsatisfied dependency expressed through field 'studyService'; nested exception is org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'studyServiceImpl': Unsatisfied dependency expressed through field 'cancerTypeService'; nested exception is org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'cancerTypeServiceImpl': Invocation of init method failed; nested exception is org.mybatis.spring.MyBatisSystemException: nested exception is org.apache.ibatis.exceptions.PersistenceException: cbioportal-container | ### Error querying database. Cause: org.springframework.jdbc.CannotGetJdbcConnectionException: Could not get JDBC Connection; nested exception is java.sql.SQLException: Cannot create PoolableConnectionFactory (Communications link failure

n1zea144 commented 5 years ago

@oplantalech If portal.properties is not present, can you show how you are passing db properties to the application? Are you passing -Dspring.profiles.active=jndi?

khzhu commented 5 years ago

@n1zea144 ,@oplantalech , we had portal.properties file that was not an issues for us. I recently successfully deployed 2.1.0 release with SAML authentication. Somehow, we still need to set

authenticate=saml

in portal.properties file and only added the following two lines to tomcat setenv.sh file:

export PORTAL_HOME=/usr/local/tomcat/webapps/cbioportal/WEB-INF/classes CATALINA_OPTS='-Ddbconnector=dbcp'

n1zea144 commented 5 years ago

For reference to turn on spring framework logging: log4j.category.org.springframework=ALL

n1zea144 commented 5 years ago

@pieterlukasse @oplantalech @khzhu I think I have a solution to the " Could not find SQL statement to include with refid 'org.cbioportal.persistence.mybatis.PatientMapper.select'" issue. Hopefully this will solve the JDBC issue upstream.

I have a general understanding of why this is happening, but I don't understand why we haven't been encountering this issue internally at MSK (more about this below).

I should note that at some point I think someone mentioned that this only happens when SAML is active, but I see it happening when any security protocol is active and not when authenticate=none. This makes sense to me as I will now explain.

I think the root of the issue is this line in applicationContext-security.xml. Notice this line is only executed when a security protocol is set. It has a cascading effect - various service classes are instantiated, the first being (in my stacktrace) StudyViewFilterApplier.java. The highlighted line in this class indirectly references SampleServiceImpl.java. The highlighted line in this class indirectly references StudyServiceImpl.java. The highlighted line here pulls in StudyMyBatisRepository.java which of course starts processing StudyMapper.xml.

If you look at this mapper file, it makes a reference to another mapping file. Here is one such example. Other mappers do the same. It is these inter-mapper file references that are causing these org.apache.ibatis.builder.IncompleteElementExceptions.

I know two things at this point -

As you can see, we load more than one context file in cBioPortal. In fact, the first file in this list is applicationContextSecurity.xml. If you recall I pointed out this file at the start of this comment - when a security protocol is set, the autowiring of various web util classes is performed in this file here. I did some googling and its not crystal clear the order in which context files are processed when more than one is passed to contextConfigLocation in web.xml.

On a hunch I moved classpath:applicationContext-business.xml to the head of the list, followed by classpath:applicationContext-security.xml. By doing this, exceptions are no longer being thrown in my test instance.

If you recall, here at MSK we are not encountering these ibatis exceptions. As a matter of fact, we use a slightly different applicationContext-security.xml file, one which allows various institutional systems backdoors into our web service. I compared our version of the security context with the one checked into the community repos and I don't see what makes ours special.

So I'm still scratching my head as to why we haven't been seeing these exceptions here. In fact, why did this issue lay dormant in the community codebase for months and only recently rear its ugly head again? Perhaps a randomness in the order in which contextConfigLocation processes multiple context files due to some subtle difference in system software versions? I don't know.

As time permits I will continue to look into this. For now, I hope changing the order of context files in the web.xml file provides the solution. I look forward to further discussion and ideas from you about this....

khzhu commented 5 years ago

thanks @n1zea144 for looking deeply into the issue and share your work with us! I will test out and let you know if the change you proposed makes any difference in our environment.

n1zea144 commented 5 years ago

@pieterlukasse @oplantalech @khzhu I think I figured out why, here at MSK we have not been encountering this issue. It may be because somehow, we've only been deploying code internally using the release (rc) branch. This branch contains code from a PR-5344 from @sheridancbio which breaks a dependency between the security module and the web module. You'll notice that the version of applicationContext-security.xml does not contain the reference to org.cbioportal.web.util that versions of this file in other branches do (it turns out our custom version of the security context file does contain a reference to the web util package, but because the underlying code does not have a dependency on web from security, that autowiring is not attempted).

I think if the rc branch is merged into master/releaseX branches, the problem should go away.

Bringing @inodb into the conversation. Maybe he can shed some light on the situation.

cc @averyniceday

oplantalech commented 5 years ago

The issue @pieterlukasse mentioned when deploying to v2.1.0 is fixed with @n1zea144's solution.

oplantalech commented 5 years ago

I have created a PR with the fix that @n1zea144 proposed: https://github.com/cBioPortal/cbioportal/pull/6012