Unverified Commit a1ce54b8 authored by CalvinKirs's avatar CalvinKirs Committed by GitHub
Browse files

[Improvement][server] Fix some sonar analyzed bugs (#3415) 

* [FIX_BUG][*]fix sonar bugs
fix assert error(assertions always pass)

* fix assert error(assertions always pass)
insert data should judging the success status

* fix assert error(assertions always pass)

* use AtomicReference<Object> to replace unsafe volatile Object

* use AtomicReference<Object> to replace unsafe volatile Object

* lazy initialization holder class Singleton...

* reformat code
judge database operations

* reformat code
judge database operations

* revert

* use ThreadLocalRandom to build random numbers

* reform code

* fix dao test error

* judge database operations
parent 056168fc

dolphinscheduler-dao/src/test/java/org/apache/dolphinscheduler/dao/mapper/AccessTokenMapperTest.java

+48 −27
Original line number Diff line number Diff line
@@ -34,6 +34,7 @@ import org.springframework.transaction.annotation.Transactional; 
import javax.annotation.Resource;

import java.text.SimpleDateFormat;

import java.util.*;

import java.util.concurrent.ThreadLocalRandom;



import static org.hamcrest.MatcherAssert.assertThat;

import static org.hamcrest.Matchers.greaterThan;
@@ -45,7 +46,7 @@ import static org.junit.Assert.*; 
@RunWith(SpringRunner.class)

@SpringBootTest

@Transactional

@Rollback(true)

@Rollback

public class AccessTokenMapperTest {



    @Autowired
@@ -56,6 +57,7 @@ public class AccessTokenMapperTest { 


    /**

     * test insert

     *

     * @throws Exception

     */

    @Test
@@ -69,6 +71,7 @@ public class AccessTokenMapperTest { 


    /**

     * test select by id

     *

     * @throws Exception

     */

    @Test
@@ -81,6 +84,7 @@ public class AccessTokenMapperTest { 


    /**

     * test hashCode method

     *

     * @throws Exception

     */

    @Test
@@ -94,6 +98,7 @@ public class AccessTokenMapperTest { 


    /**

     * test equals method

     *

     * @throws Exception

     */

    @Test
@@ -140,7 +145,10 @@ public class AccessTokenMapperTest { 
        accessToken.setToken("56789");

        accessToken.setExpireTime(DateUtils.getCurrentDate());

        accessToken.setUpdateTime(DateUtils.getCurrentDate());

        accessTokenMapper.updateById(accessToken);

        int status = accessTokenMapper.updateById(accessToken);

        if (status != 1) {

            Assert.fail("update access token fail");

        }

        AccessToken resultAccessToken = accessTokenMapper.selectById(accessToken.getId());

        assertEquals(accessToken, resultAccessToken);

    }
@@ -153,7 +161,10 @@ public class AccessTokenMapperTest { 
        Integer userId = 1;



        AccessToken accessToken = createAccessToken(userId);

        accessTokenMapper.deleteById(accessToken.getId());

        int status = accessTokenMapper.deleteById(accessToken.getId());

        if (status != 1) {

            Assert.fail("delete access token data fail");

        }



        AccessToken resultAccessToken =

                accessTokenMapper.selectById(accessToken.getId());
@@ -163,6 +174,7 @@ public class AccessTokenMapperTest { 


    /**

     * create accessTokens

     *

     * @param count    create accessToken count

     * @param userName username

     * @return accessToken map
@@ -185,6 +197,7 @@ public class AccessTokenMapperTest { 


    /**

     * create user

     *

     * @param userName userName

     * @return user

     * @throws Exception
@@ -201,36 +214,44 @@ public class AccessTokenMapperTest { 
        user.setUpdateTime(DateUtils.getCurrentDate());

        user.setQueue("default");



        userMapper.insert(user);

        int status = userMapper.insert(user);



        if (status != 1) {

            Assert.fail("insert user data error");

        }



        return user;

    }



    /**

     * create access token

     *

     * @param userId   userId

     * @param userName userName

     * @return accessToken

     * @throws Exception

     */

    private AccessToken createAccessToken(Integer userId, String userName) throws Exception {

        Random random = new Random();

        //insertOne

        AccessToken accessToken = new AccessToken();

        accessToken.setUserName(userName);

        accessToken.setUserId(userId);

        accessToken.setToken(String.valueOf(random.nextLong()));

        accessToken.setToken(String.valueOf(ThreadLocalRandom.current().nextLong()));

        accessToken.setCreateTime(DateUtils.getCurrentDate());

        accessToken.setUpdateTime(DateUtils.getCurrentDate());

        accessToken.setExpireTime(DateUtils.getCurrentDate());



        accessTokenMapper.insert(accessToken);

        int status = accessTokenMapper.insert(accessToken);



        if (status != 1) {

            Assert.fail("insert data error");

        }

        return accessToken;

    }



    /**

     * create access token

     *

     * @param userId userId

     * @return accessToken

     * @throws Exception

dolphinscheduler-dao/src/test/java/org/apache/dolphinscheduler/dao/mapper/ResourceMapperTest.java

+57 −28
Original line number Diff line number Diff line
@@ -65,6 +65,7 @@ public class ResourceMapperTest { 


    /**

     * insert

     *

     * @return Resource

     */

    private Resource insertOne() {
@@ -76,12 +77,16 @@ public class ResourceMapperTest { 
        resource.setDirectory(false);

        resource.setType(ResourceType.FILE);

        resource.setUserId(111);

        resourceMapper.insert(resource);

        int status = resourceMapper.insert(resource);

        if (status != 1) {

            Assert.fail("insert data error");

        }

        return resource;

    }



    /**

     * create resource by user

     *

     * @param user user

     * @return Resource

     */
@@ -93,12 +98,16 @@ public class ResourceMapperTest { 
        resource.setAlias(alias);

        resource.setFullName(fullName);

        resource.setUserId(user.getId());

        resourceMapper.insert(resource);

        int status = resourceMapper.insert(resource);

        if (status != 1) {

            Assert.fail("insert data error");

        }

        return resource;

    }



    /**

     * create resource by user

     *

     * @param user user

     * @return Resource

     */
@@ -113,6 +122,7 @@ public class ResourceMapperTest { 


    /**

     * create user

     *

     * @return User

     */

    private User createGeneralUser(String userName) {
@@ -124,12 +134,17 @@ public class ResourceMapperTest { 
        user.setCreateTime(new Date());

        user.setTenantId(1);

        user.setUpdateTime(new Date());

        userMapper.insert(user);

        int status = userMapper.insert(user);



        if (status != 1) {

            Assert.fail("insert data error");

        }

        return user;

    }



    /**

     * create resource user

     *

     * @return ResourcesUser

     */

    private ResourcesUser createResourcesUser(Resource resource, User user) {
@@ -150,6 +165,7 @@ public class ResourceMapperTest { 
        assertNotNull(resource.getId());

        assertThat(resource.getId(), greaterThan(0));

    }



    /**

     * test update

     */
@@ -294,16 +310,28 @@ public class ResourceMapperTest { 
        Tenant tenant = new Tenant();

        tenant.setTenantName("ut tenant ");

        tenant.setTenantCode("ut tenant code for resource");

        tenantMapper.insert(tenant);

        int tenantInsertStatus = tenantMapper.insert(tenant);



        if (tenantInsertStatus != 1) {

            Assert.fail("insert tenant data error");

        }



        User user = new User();

        user.setTenantId(tenant.getId());

        user.setUserName("ut user");

        userMapper.insert(user);

        int userInsertStatus = userMapper.insert(user);



        if (userInsertStatus != 1) {

            Assert.fail("insert user data error");

        }





        Resource resource = insertOne();

        resource.setUserId(user.getId());

        resourceMapper.updateById(resource);

        int userUpdateStatus = resourceMapper.updateById(resource);

        if (userUpdateStatus != 1) {

            Assert.fail("update user data error");

        }



        String resource1 = resourceMapper.queryTenantCodeByResourceName(

                resource.getFullName(), ResourceType.FILE.ordinal()
@@ -332,7 +360,6 @@ public class ResourceMapperTest { 
        Assert.assertFalse(resources.stream().map(t -> t.getFullName()).collect(toList()).containsAll(Arrays.asList(resNames)));







        // authorize object unauthorizedResource to generalUser

        createResourcesUser(unauthorizedResource, generalUser2);

        List<Resource> authorizedResources = resourceMapper.listAuthorizedResource(generalUser2.getId(), resNames);
@@ -382,6 +409,8 @@ public class ResourceMapperTest { 
        List<Resource> resourceList = new ArrayList<>();

        resourceList.add(resource);

        int result = resourceMapper.batchUpdateResource(resourceList);

        Assert.assertTrue(result>0);

        if (result != resourceList.size()) {

            Assert.fail("batch update resource  data error");

        }

    }

}
 
 No newline at end of file

dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/future/TaskFuture.java

+11 −10
Original line number Diff line number Diff line
@@ -28,6 +28,7 @@ import java.util.Map; 
import java.util.concurrent.ConcurrentHashMap;

import java.util.concurrent.CountDownLatch;

import java.util.concurrent.TimeUnit;

import java.util.concurrent.atomic.AtomicReference;



/**

 *  task fulture
@@ -55,11 +56,11 @@ public class TaskFuture { 
    /**

     *  response command

     */

    private volatile Command responseCommand;

    private  AtomicReference<Command> responseCommandReference = new AtomicReference<>();



    private volatile boolean sendOk = true;



    private volatile Throwable cause;

    private  AtomicReference<Throwable> causeReference;



    public TaskFuture(long opaque, long timeoutMillis) {

        this.opaque = opaque;
@@ -74,7 +75,7 @@ public class TaskFuture { 
     */

    public Command waitResponse() throws InterruptedException {

        this.latch.await(timeoutMillis, TimeUnit.MILLISECONDS);

        return this.responseCommand;

        return this.responseCommandReference.get();

    }



    /**
@@ -83,7 +84,7 @@ public class TaskFuture { 
     * @param responseCommand responseCommand

     */

    public void putResponse(final Command responseCommand) {

        this.responseCommand = responseCommand;

        responseCommandReference.set(responseCommand);

        this.latch.countDown();

        FUTURE_TABLE.remove(opaque);

    }
@@ -114,11 +115,11 @@ public class TaskFuture { 
    }



    public void setCause(Throwable cause) {

        this.cause = cause;

        causeReference.set(cause);

    }



    public Throwable getCause() {

        return cause;

        return causeReference.get();

    }



    public long getOpaque() {
@@ -134,11 +135,11 @@ public class TaskFuture { 
    }



    public Command getResponseCommand() {

        return responseCommand;

        return responseCommandReference.get();

    }



    public void setResponseCommand(Command responseCommand) {

        this.responseCommand = responseCommand;

        responseCommandReference.set(responseCommand);

    }
@@ -166,9 +167,9 @@ public class TaskFuture { 
                ", timeoutMillis=" + timeoutMillis +

                ", latch=" + latch +

                ", beginTimestamp=" + beginTimestamp +

                ", responseCommand=" + responseCommand +

                ", responseCommand=" + responseCommandReference.get() +

                ", sendOk=" + sendOk +

                ", cause=" + cause +

                ", cause=" + causeReference.get() +

                '}';

    }

}

dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/processor/queue/TaskResponseService.java

+1 −0
Original line number Diff line number Diff line
@@ -108,6 +108,7 @@ public class TaskResponseService { 
                    TaskResponseEvent taskResponseEvent = eventQueue.take();

                    persist(taskResponseEvent);

                } catch (InterruptedException e){

                    Thread.currentThread().interrupt();

                    break;

                } catch (Exception e){

                    logger.error("persist task error",e);

dolphinscheduler-service/src/main/java/org/apache/dolphinscheduler/service/quartz/QuartzExecutors.java

+6 −17
Original line number Diff line number Diff line
@@ -61,20 +61,20 @@ public class QuartzExecutors { 
   */

  private static Scheduler scheduler;



  /**

   * instance of QuartzExecutors

   */

  private static volatile QuartzExecutors INSTANCE = null;



  /**

   * load conf

   */

  private static Configuration conf;



  private static final class Holder {

    private static final QuartzExecutors instance = new QuartzExecutors();

  }





  private QuartzExecutors() {

    try {

      conf = new PropertiesConfiguration(QUARTZ_PROPERTIES_PATH);

      init();

    }catch (ConfigurationException e){

      logger.warn("not loaded quartz configuration file, will used default value",e);

    }
@@ -85,18 +85,7 @@ public class QuartzExecutors { 
   * @return instance of Quartz Executors

   */

  public static QuartzExecutors getInstance() {

    if (INSTANCE == null) {

      synchronized (QuartzExecutors.class) {

        // when more than two threads run into the first null check same time, to avoid instanced more than one time, it needs to be checked again.

        if (INSTANCE == null) {

          QuartzExecutors quartzExecutors = new QuartzExecutors();

          //finish QuartzExecutors init

          quartzExecutors.init();

          INSTANCE = quartzExecutors;

        }

      }

    }

    return INSTANCE;

   return Holder.instance;

  }