feat(open-api): get authorized apps #3647

Anilople · 2021-04-24T06:40:38Z

What's the purpose of this PR

Let open api know that which applications it can operate.

Which issue(s) this PR fixes:

Relate to #3607

Brief changelog

Add a new controller in portal

Add a new method in ApolloOpenApiClient

Follow this checklist to help us incorporate your contribution quickly and easily:

Read the Contributing Guide before making this pull request.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Write necessary unit tests to verify the code.
Run mvn clean test to make sure this pull request doesn't break anything.

...lo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/AuthorizationService.java

codecov-commenter · 2021-04-24T06:44:55Z

Codecov Report

❗ No coverage uploaded for pull request base (master@9a6f8e2). Click here to learn what that means.
The diff coverage is 55.43%.

@@            Coverage Diff            @@
##             master    #3647   +/-   ##
=========================================
  Coverage          ?   50.12%           
  Complexity        ?     2442           
=========================================
  Files             ?      479           
  Lines             ?    14810           
  Branches          ?     1504           
=========================================
  Hits              ?     7424           
  Misses            ?     6865           
  Partials          ?      521

Impacted Files	Coverage Δ
...k/apollo/adminservice/AdminServiceApplication.java	`0.00% <ø> (ø)`
...mework/apollo/adminservice/ServletInitializer.java	`0.00% <ø> (ø)`
...o/adminservice/aop/NamespaceAcquireLockAspect.java	`70.17% <ø> (ø)`
.../apollo/adminservice/controller/AppController.java	`86.20% <ø> (ø)`
...dminservice/controller/AppNamespaceController.java	`34.61% <ø> (ø)`
...llo/adminservice/controller/ClusterController.java	`56.00% <ø> (ø)`
...ollo/adminservice/controller/CommitController.java	`60.00% <ø> (ø)`
...pollo/adminservice/controller/IndexController.java	`50.00% <ø> (ø)`
...inservice/controller/InstanceConfigController.java	`92.77% <ø> (ø)`
...llo/adminservice/controller/ItemSetController.java	`100.00% <ø> (ø)`
... and 124 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a6f8e2...5a999dc. Read the comment docs.

nobodyiam

besides, it's better to add necessary unit tests for the new logic

.../src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/AuthorizationController.java

...lo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/AuthorizationService.java

github-actions · 2021-05-15T13:03:54Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

JaredTan95 · 2021-06-06T07:24:33Z

as #3747 merged, please update changs log. :-)

nobodyiam · 2021-06-13T05:45:33Z

@Anilople any update on this?

Anilople · 2021-06-16T14:29:21Z

besides, it's better to add necessary unit tests for the new logic

Is there a better way to test it? It is a little hard, I don't know how to write test for it.

nobodyiam · 2021-06-18T00:27:04Z

besides, it's better to add necessary unit tests for the new logic

Is there a better way to test it? It is a little hard, I don't know how to write test for it.

I notice you added an integration test in com.ctrip.framework.apollo.openapi.v1.controller.AppControllerTest, this looks okay to me. However, in integration test, normally we wouldn't mock the main logic and would set up the data instead, you may refer the example com.ctrip.framework.apollo.configservice.integration.ConfigControllerIntegrationTest for more details.
And in this case, I think a simple unit test is also okay, e.g. after mocking the necessary dependencies, we simple call appController.findAppsAuthorized and verify the execution logic and result. There is also an example for your reference: com.ctrip.framework.apollo.configservice.controller.ConfigControllerTest.

BTW, as @JaredTan95 mentioned, please also update the CHANGES.md.

…ervice

Anilople · 2021-06-20T02:28:52Z

...src/test/java/com/ctrip/framework/apollo/openapi/service/ConsumerServiceIntegrationTest.java

+  private ConsumerService consumerService;
+
+  @Test
+  @Sql(scripts = "ConsumerServiceIntegrationTest.testFindAppIdsAuthorizedByConsumerId.sql", executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD)


I dump the part of data as sql from portaldb. Is it a good way to prepare data for test?

The data looks ok, however I have some suggestions:

The delete statements are not necessary since they will be cleared via /sql/cleanup.sql

To test the logic, it's better to add more apps, e.g. consumer-test-app-id-0, consumer-test-app-id-1 and consumer-test-app-id-2. And we setup the data that the consumer only has permission to consumer-test-app-id-0 and consumer-test-app-id-1.

For integration test, I would suggest we test against the open api /apps/authorized, e.g using a rest template to request the path

Then the AppController#findAppsAuthorized and ConsumerService#findAppIdsAuthorizedByConsumerId could be tested by pure unit tests, which doesn't need to start a server

Anilople · 2021-06-21T13:38:30Z

...test/java/com/ctrip/framework/apollo/openapi/v1/controller/AppControllerIntegrationTest.java

+  public void testFindAppsAuthorized() {
+    final String token = "3c16bf5b1f44b465179253442460e8c0ad845289";
+    HttpHeaders httpHeaders = new HttpHeaders();
+    httpHeaders.set(HttpHeaders.AUTHORIZATION, token);


Is it ok to set token by this way?

I think it's ok

btw, is the following sql necessary? I think every test should clean its data after execution, so there is no need to clear the data before execution?

@Sql(scripts = "/sql/cleanup.sql", executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD)

When we delete the cleanup before run this test, code chagne to

@Test // @Sql(scripts = "/sql/cleanup.sql", executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD) @Sql(scripts = "/com/ctrip/framework/apollo/openapi/service/ConsumerServiceIntegrationTest.testFindAppIdsAuthorizedByConsumerId.sql", executionPhase = Sql.ExecutionPhase.BEFORE_TEST_METHOD) @Sql(scripts = "/sql/cleanup.sql", executionPhase = Sql.ExecutionPhase.AFTER_TEST_METHOD) public void testFindAppsAuthorized() {

and run a single test AppControllerIntegrationTest, will meet error org.h2.jdbc.JdbcSQLException: Unique index or primary key violation: "PRIMARY KEY ON PUBLIC.PERMISSION(ID)";

full log as follow

org.springframework.jdbc.datasource.init.ScriptStatementFailedException: Failed to execute SQL script statement #12 of class path resource [com/ctrip/framework/apollo/openapi/service/ConsumerServiceIntegrationTest.testFindAppIdsAuthorizedByConsumerId.sql]: INSERT INTO `Permission` (`Id`, `PermissionType`, `TargetId`, `DataChange_CreatedBy`, `DataChange_LastModifiedBy`) VALUES (1, 'AssignRole', 'consumer-test-app-id-0', 'apollo', 'apollo'); nested exception is org.h2.jdbc.JdbcSQLException: Unique index or primary key violation: "PRIMARY KEY ON PUBLIC.PERMISSION(ID)"; SQL statement: INSERT INTO `Permission` (`Id`, `PermissionType`, `TargetId`, `DataChange_CreatedBy`, `DataChange_LastModifiedBy`) VALUES (1, 'AssignRole', 'consumer-test-app-id-0', 'apollo', 'apollo') [23505-191] at org.springframework.jdbc.datasource.init.ScriptUtils.executeSqlScript(ScriptUtils.java:622) at org.springframework.jdbc.datasource.init.ResourceDatabasePopulator.populate(ResourceDatabasePopulator.java:254) at org.springframework.jdbc.datasource.init.DatabasePopulatorUtils.execute(DatabasePopulatorUtils.java:49) at org.springframework.jdbc.datasource.init.ResourceDatabasePopulator.execute(ResourceDatabasePopulator.java:269) at org.springframework.test.context.jdbc.SqlScriptsTestExecutionListener.lambda$executeSqlScripts$4(SqlScriptsTestExecutionListener.java:278) at org.springframework.transaction.support.TransactionOperations.lambda$executeWithoutResult$0(TransactionOperations.java:68) at org.springframework.transaction.support.TransactionTemplate.execute(TransactionTemplate.java:140) at org.springframework.transaction.support.TransactionOperations.executeWithoutResult(TransactionOperations.java:67) at org.springframework.test.context.jdbc.SqlScriptsTestExecutionListener.executeSqlScripts(SqlScriptsTestExecutionListener.java:278) at org.springframework.test.context.jdbc.SqlScriptsTestExecutionListener.lambda$executeSqlScripts$0(SqlScriptsTestExecutionListener.java:200) at java.lang.Iterable.forEach(Iterable.java:75) at org.springframework.test.context.jdbc.SqlScriptsTestExecutionListener.executeSqlScripts(SqlScriptsTestExecutionListener.java:200) at org.springframework.test.context.jdbc.SqlScriptsTestExecutionListener.executeSqlScripts(SqlScriptsTestExecutionListener.java:144) at org.springframework.test.context.jdbc.SqlScriptsTestExecutionListener.beforeTestMethod(SqlScriptsTestExecutionListener.java:117) at org.springframework.test.context.TestContextManager.beforeTestMethod(TestContextManager.java:289) at org.springframework.test.context.junit4.statements.RunBeforeTestMethodCallbacks.evaluate(RunBeforeTestMethodCallbacks.java:74) at org.springframework.test.context.junit4.statements.RunAfterTestMethodCallbacks.evaluate(RunAfterTestMethodCallbacks.java:86) at org.springframework.test.context.junit4.statements.SpringRepeat.evaluate(SpringRepeat.java:84) at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366) at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:251) at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:97) at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329) at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293) at org.springframework.test.context.junit4.statements.RunBeforeTestClassCallbacks.evaluate(RunBeforeTestClassCallbacks.java:61) at org.springframework.test.context.junit4.statements.RunAfterTestClassCallbacks.evaluate(RunAfterTestClassCallbacks.java:70) at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306) at org.junit.runners.ParentRunner.run(ParentRunner.java:413) at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.run(SpringJUnit4ClassRunner.java:190) at org.junit.runner.JUnitCore.run(JUnitCore.java:137) at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:69) at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:33) at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:221) at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:54) Caused by: org.h2.jdbc.JdbcSQLException: Unique index or primary key violation: "PRIMARY KEY ON PUBLIC.PERMISSION(ID)"; SQL statement: INSERT INTO `Permission` (`Id`, `PermissionType`, `TargetId`, `DataChange_CreatedBy`, `DataChange_LastModifiedBy`) VALUES (1, 'AssignRole', 'consumer-test-app-id-0', 'apollo', 'apollo') [23505-191] at org.h2.message.DbException.getJdbcSQLException(DbException.java:345) at org.h2.message.DbException.get(DbException.java:179) at org.h2.message.DbException.get(DbException.java:155) at org.h2.mvstore.db.MVPrimaryIndex.add(MVPrimaryIndex.java:137) at org.h2.mvstore.db.MVTable.addRow(MVTable.java:704) at org.h2.command.dml.Insert.insertRows(Insert.java:156) at org.h2.command.dml.Insert.update(Insert.java:114) at org.h2.command.CommandContainer.update(CommandContainer.java:98) at org.h2.command.Command.executeUpdate(Command.java:258) at org.h2.jdbc.JdbcStatement.executeInternal(JdbcStatement.java:184) at org.h2.jdbc.JdbcStatement.execute(JdbcStatement.java:158) at com.zaxxer.hikari.pool.ProxyStatement.execute(ProxyStatement.java:95) at com.zaxxer.hikari.pool.HikariProxyStatement.execute(HikariProxyStatement.java) at org.springframework.jdbc.datasource.init.ScriptUtils.executeSqlScript(ScriptUtils.java:601) ... 35 more

right, there is an initialization logic in com.ctrip.framework.apollo.portal.spi.defaultimpl.DefaultRoleInitializationService#initCreateAppRole which will create one set of role, permission and role permission record. I think we could change the id in the test sql.

btw, is it better to organize the sql file into the sql folder?

Have been moved to folder resources/sql/openapi

nobodyiam

LGTM

nobodyiam

LGTM

feat(open-api): get authorized apps

ecda570

Anilople commented Apr 24, 2021

View reviewed changes

...lo-portal/src/main/java/com/ctrip/framework/apollo/openapi/service/AuthorizationService.java Outdated Show resolved Hide resolved

Merge branch 'master' into feature/openapi/enhancement/20210417

29fa187

Anilople requested review from kezhenxu94, nisiyong and nobodyiam April 24, 2021 06:45

Anilople added area/openapi apollo-openapi feature Categorizes issue as related to a new feature. labels Apr 24, 2021

nobodyiam requested changes Apr 27, 2021

View reviewed changes

Merge branch 'master' into feature/openapi/enhancement/20210417

1c1522d

nobodyiam added this to the 1.9.0 milestone May 23, 2021

Anilople and others added 6 commits June 13, 2021 18:12

Merge branch 'master' into feature/openapi/enhancement/20210417

f0931c6

refactor: delete new class

4d41625

fix: test failure

f3adccd

rename method name

472dd45

Merge branch 'master' into feature/openapi/enhancement/20210417

5cacf99

Merge branch 'master' into feature/openapi/enhancement/20210417

73f4c01

Anilople added 2 commits June 16, 2021 23:33

fix: extractAppIdFromMasterRoleName -> extractAppIdFromRoleName

c1d7cbd

test: find app authorized

f9cdb84

Anilople and others added 4 commits June 19, 2021 20:13

Merge branch 'master' into feature/openapi/enhancement/20210417

1d45338

Update CHANGES.md

f4d182b

refactor: create method findAppIdsAuthorizedByConsumerId in ConsumerS…

11d39b3

…ervice

test: use sql to prepare data

1ddc81c

Anilople requested a review from nobodyiam June 20, 2021 02:26

Anilople commented Jun 20, 2021

View reviewed changes

Anilople added 2 commits June 21, 2021 20:53

test: add more app consumer-test-app-id-2

348af40

test: restTemplate call api with token

99822cb

Anilople commented Jun 21, 2021

View reviewed changes

Anilople added 2 commits June 25, 2021 19:21

test: append 000 to all id in sql

dea5f3c

test: move sql to folder /sql/openapi

c284ae5

nobodyiam previously approved these changes Jun 26, 2021

View reviewed changes

Merge branch 'master' into feature/openapi/enhancement/20210417

5a999dc

nobodyiam dismissed their stale review via 5a999dc June 26, 2021 13:14

nobodyiam approved these changes Jun 26, 2021

View reviewed changes

nobodyiam merged commit f1eca02 into apolloconfig:master Jun 26, 2021

github-actions bot locked and limited conversation to collaborators Jun 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(open-api): get authorized apps #3647

feat(open-api): get authorized apps #3647

Anilople commented Apr 24, 2021

codecov-commenter commented Apr 24, 2021 •

edited

Loading

nobodyiam left a comment

github-actions bot commented May 15, 2021 •

edited

Loading

JaredTan95 commented Jun 6, 2021

nobodyiam commented Jun 13, 2021

Anilople commented Jun 16, 2021

nobodyiam commented Jun 18, 2021

Anilople Jun 20, 2021 •

edited

Loading

nobodyiam Jun 20, 2021 •

edited

Loading

Anilople Jun 21, 2021

nobodyiam Jun 23, 2021

Anilople Jun 23, 2021

nobodyiam Jun 25, 2021

nobodyiam Jun 25, 2021

Anilople Jun 25, 2021

nobodyiam left a comment

nobodyiam left a comment

feat(open-api): get authorized apps #3647

feat(open-api): get authorized apps #3647

Conversation

Anilople commented Apr 24, 2021

What's the purpose of this PR

Which issue(s) this PR fixes:

Brief changelog

codecov-commenter commented Apr 24, 2021 • edited Loading

Codecov Report

nobodyiam left a comment

Choose a reason for hiding this comment

github-actions bot commented May 15, 2021 • edited Loading

JaredTan95 commented Jun 6, 2021

nobodyiam commented Jun 13, 2021

Anilople commented Jun 16, 2021

nobodyiam commented Jun 18, 2021

Anilople Jun 20, 2021 • edited Loading

Choose a reason for hiding this comment

nobodyiam Jun 20, 2021 • edited Loading

Choose a reason for hiding this comment

Anilople Jun 21, 2021

Choose a reason for hiding this comment

nobodyiam Jun 23, 2021

Choose a reason for hiding this comment

Anilople Jun 23, 2021

Choose a reason for hiding this comment

nobodyiam Jun 25, 2021

Choose a reason for hiding this comment

nobodyiam Jun 25, 2021

Choose a reason for hiding this comment

Anilople Jun 25, 2021

Choose a reason for hiding this comment

nobodyiam left a comment

Choose a reason for hiding this comment

nobodyiam left a comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 24, 2021 •

edited

Loading

github-actions bot commented May 15, 2021 •

edited

Loading

Anilople Jun 20, 2021 •

edited

Loading

nobodyiam Jun 20, 2021 •

edited

Loading