feat: Add apollo audit log common solution backend #4958
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
import apollo-audit-api in biz module, but register that 'api' bean in AdminService module when ApolloAuditAutoConfiguration auto configure. I guess that's the reason of the test failures. |
scripts/sql/apolloconfigdb.sql
Outdated
| `FieldNewValue` varchar(500) DEFAULT NULL COMMENT '字段新值', | ||
| PRIMARY KEY (`Id`), | ||
| INDEX `SpanIdIndex` (`SpanId`) | ||
| ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COMMENT='审计日志数据变动表'; |
There was a problem hiding this comment.
- think that how user will query by ui
- from user's query, how many http api we need to provide, and what there are. include request, response
- according the http api's request, try to add index to the table.
key word: page, filter by time, find by traceId, filter by OpName
pom.xml
Outdated
| <dependency> | ||
| <groupId>com.ctrip.framework.apollo.apollo-audit</groupId> | ||
| <artifactId>apollo-audit-spring-boot-starter</artifactId> | ||
| <version>${revision}</version> |
There was a problem hiding this comment.
| <version>${revision}</version> | |
| <version>${project.version}</version> |
revision here will cause some problem to maven, use project.version instead of it.
Anilople
changed the title
|
Illustrate that how to use this module by the config and annotation |
Codecov Report
@@ Coverage Diff @@
## master #4958 +/- ##
============================================
- Coverage 48.48% 48.39% -0.10%
+ Complexity 1726 1724 -2
============================================
Files 346 347 +1
Lines 10836 10848 +12
Branches 1080 1080
============================================
- Hits 5254 5250 -4
- Misses 5258 5273 +15
- Partials 324 325 +1
|
to explain what this feature will bring to the persistence, this feature consists of two entity classes, AuditLog and AuditLogDataInfluence.
now its easier to illustrate that how to use this module by the config and annotation/api: config side:Currently, only one configuration option is available, When adding the if else then annotation/api side:
Added the How Annotation and Api align
└── annotation
├── ApolloAuditLog.java
├── ApolloAuditLogDataInfluence.java
├── ApolloAuditLogDataInfluenceTable.java
├── ApolloAuditLogDataInfluenceTableField.java
├── ApolloAuditLogDataInfluenceTableId.java
└── OpType.java
└── api
├── ApolloAuditEntityWrapper.java
└── ApolloAuditLogApi.javapublic interface ApolloAuditLogApi {
AutoCloseable appendSpan(OpType type, String name, String description);
void appendSingleDataInfluence(String entityId, String entityName, String fieldName,
String fieldOldValue, String fieldNewValue);
<T> void appendDataInfluencesByManagedClass(List<T> entities, OpType type, Class<?> managedClass);
<T> void appendDataInfluencesByWrapper(List<T> entities, OpType type,
ApolloAuditEntityWrapper wrapper);
}
public class ApolloAuditEntityWrapper {
private String entityName;
private List<Field> dataInfluenceFields;
private Field entityIdField;
...
}At the same time, an For trace id, span id, scope and else, mentor told me those are the internal complexity of the framework itself, which does not want to be perceived externally. So I design like the code shows below, completly use api manually. public void delete(long id, String operator) {
App app = appRepository.findById(id).orElse(null);
if (app == null) {
return;
}
try(AutoCloseable auditScope = apolloAuditLogApi.appendSpan(OpType.DELETE, "app.delete")) {
app.setDeleted(true);
app.setDataChangeLastModifiedBy(operator);
appRepository.save(app);
ApolloAuditEntityWrapper wrapper = new ApolloAuditEntityWrapper();
wrapper.entityName("App")
.entityIdField(app.getClass().getDeclaredField("appId"))
.addDataInfluenceFields(app.getClass().getDeclaredField("name"));
apolloAuditLogApi.appendDataInfluencesByWrapper(Collections.singletonList(app),OpType.DELETE, wrapper);
auditService.audit(App.class.getSimpleName(), id, Audit.OP.DELETE, operator);
} catch (Exception e) {
throw new RuntimeException(e);
}
} |
scripts/sql/apolloconfigdb.sql
Outdated
|
|
||
| DROP TABLE IF EXISTS `AuditLog`; | ||
|
|
||
| CREATE TABLE `AuditLog` ( |
There was a problem hiding this comment.
@apolloconfig/committers For backward compability as more as possible
The solution is that
- create new table, the new audit log only save in the new table.
- open this feature by config
apollo.audit.log.enabled=true, and user can close it by change toapollo.audit.log.enabled=false - create new maven module apollo-audit, other modules use this feature by annotation or interface
ApolloAuditLogApi.
Is it ok?
There was a problem hiding this comment.
here used @EnableJpaRepositories to manually scan repositories before, but it is actually unnecessary and will affect the scanning of repositories in my audit module.
The JpaRepositoriesAutoConfiguration autoconfiguration class itself automatically scans all packages under the startup classpath. So without this annotation, it can also scan the original repositories.
Therefore, the note itself can be deleted.
There was a problem hiding this comment.
I'm so so so sorry because when I formatted these test classes, I seem to have changed a lot of the original format...
There was a problem hiding this comment.
In fact, the things I changed with the test module were the deletion of unnecessary annotations and the addition of statements to the properties file to start the audit module
There was a problem hiding this comment.
git checkout ${commitId} -- ${file relative path}
use a old commitId, the file will change to old content
|
I haven't add many uses of the audit module to other modules, for the reason that it seems decrease the test covery, so what should I do? Add the annotation/api uses in another pull request? |
|
For this PR, what else needs to be done and what needs to be improved？ :) |
| } | ||
|
|
||
| @GetMapping("/logs/data_influences/entity_name/{entityName}/entity_id/{entityId}") | ||
| public List<ApolloAuditLogDataInfluence> findDataInfluencesByEntity( |
There was a problem hiding this comment.
data_influences -> dataInfluences, delete _
|
|
||
| @GetMapping("/logs/data_influences/entity_name/{entityName}/entity_id/{entityId}") | ||
| public List<ApolloAuditLogDataInfluence> findDataInfluencesByEntity( | ||
| @PathVariable String entityName, @PathVariable String entityId, Pageable page) { |
There was a problem hiding this comment.
@PathVariable String entityName -> @RequestParam String entityName
| OpType type(); | ||
| String name(); | ||
| String description() default "no description"; | ||
| boolean autoLog() default false; |
There was a problem hiding this comment.
use ApolloAuditLogDataInfluence instead
|
|
||
| @Target(ElementType.FIELD) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| public @interface ApolloAuditLogDataInfluenceTableId { |
There was a problem hiding this comment.
delete it, index by traceId finally
apollo-biz/pom.xml
Outdated
| </dependency> | ||
| <dependency> | ||
| <groupId>com.ctrip.framework.apollo</groupId> | ||
| <artifactId>apollo-audit-spring-boot-starter</artifactId> |
There was a problem hiding this comment.
Only use apollo-audit-api?
There was a problem hiding this comment.
Only use apollo-audit-api?
when running biz-tests, it need to register bean of ApolloAuditLogApi which is provide by auto-configuration. So the dependency's scope is test to making tests pass.
we don't run biz individually, so yes we only need apollo-audit-api. But in tests, we run it individually, so it needs starter for starting app.
There was a problem hiding this comment.
git checkout ${commitId} -- ${file relative path}
use a old commitId, the file will change to old content
| import com.ctrip.framework.apollo.audit.annotation.OpType; | ||
| import java.util.List; | ||
|
|
||
| public interface ApolloAuditLogApi { |
There was a problem hiding this comment.
Controller use ApolloAuditLogApi
There was a problem hiding this comment.
Maybe create 2 api
- ApolloAuditLogRecordApi
- ApolloAuditLogQueryApi (for user's query)
then let ApolloAuditLogApi extends them
| return logRepository.findAll(pageable).getContent(); | ||
| } | ||
|
|
||
| public List<ApolloAuditLog> findByOpType(String opType, Pageable page) { |
There was a problem hiding this comment.
delete this method? only keep findByOpName?
scripts/sql/apolloportaldb.sql
Outdated
| `DataChange_CreatedTime` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP COMMENT '创建时间', | ||
| `DataChange_LastModifiedBy` varchar(64) DEFAULT '' COMMENT '最后修改人邮箱前缀', | ||
| `DataChange_LastTime` timestamp NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP COMMENT '最后修改时间', | ||
| PRIMARY KEY (`Id`) |
There was a problem hiding this comment.
KEY DataChange_LastTime (DataChange_LastTime),
scripts/sql/apolloconfigdb.sql
Outdated
| `FieldNewValue` varchar(500) DEFAULT NULL COMMENT '字段新值', | ||
| PRIMARY KEY (`Id`), | ||
| INDEX `SpanIdIndex` (`SpanId`) | ||
| ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COMMENT='审计日志数据变动表'; |
There was a problem hiding this comment.
- think that how user will query by ui
- from user's query, how many http api we need to provide, and what there are. include request, response
- according the http api's request, try to add index to the table.
key word: page, filter by time, find by traceId, filter by OpName
|
|
||
| <dependency> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-data-jpa</artifactId> |
There was a problem hiding this comment.
Why add jpa here? Need annotation?
There was a problem hiding this comment.
yes, for entities annotations.Should replaced by
<dependency>
<groupId>jakarta.persistence</groupId>
<artifactId>jakarta.persistence-api</artifactId>
</dependency>
<dependency>
<groupId>org.hibernate</groupId>
<artifactId>hibernate-core</artifactId>
</dependency>
?
|
|
||
| public interface ApolloAuditLogQueryApi { | ||
|
|
||
| List<ApolloAuditLog> queryAllLogs(Pageable page); |
There was a problem hiding this comment.
| List<ApolloAuditLog> queryAllLogs(Pageable page); | |
| List<ApolloAuditLog> queryLogs(Pageable page); |
|
|
||
| public class DaoApolloAuditLogApi implements ApolloAuditLogApi { | ||
| public class JpaApolloAuditLogApi implements ApolloAuditLogApi { |
There was a problem hiding this comment.
| public class JpaApolloAuditLogApi implements ApolloAuditLogApi { | |
| public class ApolloAuditLogApiJpaImpl implements ApolloAuditLogApi { |
|
|
||
| void appendDataInfluenceWrapper(Class<?> clazz); | ||
|
|
||
| void appendDataInfluenceWrapper(Class<?> clazz, ApolloAuditEntityWrapper wrapper); |
There was a problem hiding this comment.
If someone use ApolloAuditLogRecordApi, appendDataInfluenceWrapper should be notice?
Can we append the DataInfluence directly?
i.e append the DataInfluence and the definition class in a method.
| @EventListener | ||
| public void handleEvent(ApolloAuditLogDataInfluenceEvent event) { | ||
| Object e = event.getEntity(); | ||
| api.appendDataInfluences(Collections.singletonList(e), event.isDeleted(), e.getClass()); |
| @@ -87,14 +86,8 @@ public Object around(ProceedingJoinPoint pjp, ApolloAuditLog auditLog) throws Th | |||
|
|
|||
| try (AutoCloseable scope = api.appendSpan(auditLog.type(), auditLog.name(), | |||
| auditLog.description())) { | |||
| dataInfluenceList.forEach(e -> api.appendDataInfluenceWrapper(e.getClass())); | |||
| returnVal = pjp.proceed(); | |||
There was a problem hiding this comment.
Can we append DataInfluence after method finished?
Maybe after pjp.proceed() finished, just call a method in api will be more simpler?
for example
returnVal = pjp.proceed();
Class<?> xxxEntity = // get from method signaure, argument with annotation
api.appendDataInfluenceList(xxxEntity, dataInfluenceList);and it work will without appendDataInfluenceWrapper?
| @@ -22,3 +22,5 @@ | |||
|
|
|||
| # You may change the following config to activate different database profiles like h2/postgres | |||
| spring.profiles.group.github = mysql | |||
|
|
|||
| apollo.audit.log.enabled = false | |||
There was a problem hiding this comment.
| apollo.audit.log.enabled = false | |
| # true: enabled the new feature of audit log | |
| # false: disable it | |
| apollo.audit.log.enabled = false |
| import org.springframework.context.annotation.Configuration; | ||
|
|
||
| @Configuration | ||
| public class ApolloAuditConfiguration { |
There was a problem hiding this comment.
Can we move ApolloAuditSpanService and ApolloAuditConfiguration to audit-log module?
The logic in portal or admin service are equal? i.e always add some information to RequestContextHolder.getRequestAttributes(); if those information don't exist.
|
|
||
| /** | ||
| * http header constants | ||
| * @author luke ([email protected]) |
| * http header constants | ||
| * @author luke ([email protected]) | ||
| */ | ||
| public interface ApolloAuditHttpHeader { |
There was a problem hiding this comment.
| public interface ApolloAuditHttpHeader { | |
| public interface ApolloAuditHttpHeaderContants { |
| public ApolloAuditScopeManager() { | ||
| } | ||
|
|
||
| public ApolloAuditScope activate(ApolloAuditSpanContext spanContext) { |
There was a problem hiding this comment.
If two thread invoke the activate method, the field will be overwrite.
| </dependency> | ||
| <dependency> | ||
| <groupId>org.aspectj</groupId> | ||
| <artifactId>aspectjrt</artifactId> |
There was a problem hiding this comment.
Why need aspectj here? the aop?
| @Bean | ||
| @ConditionalOnMissingBean(ApolloAuditHttpInterceptor.class) | ||
| public ApolloAuditHttpInterceptor apolloAuditLogHttpTracerInterceptor(ApolloAuditLogApi api) { | ||
| return new ApolloAuditHttpInterceptor(api); |
There was a problem hiding this comment.
- NoOpApolloAuditHttpInterceptor?
| @@ -22,3 +22,5 @@ | |||
|
|
|||
| # You may change the following config to activate different database profiles like h2/postgres | |||
| spring.profiles.group.github = mysql | |||
|
|
|||
| apollo.audit.log.enabled = false | |||
There was a problem hiding this comment.
Add comment like admin service for user
| @@ -587,4 +592,9 @@ public ServerConfig createOrUpdateConfigDBConfig(Env env, ServerConfig serverCon | |||
| } | |||
| } | |||
|
|
|||
| @Service | |||
| public static class AuditLogAPI extends API { | |||
There was a problem hiding this comment.
Can we delete this class?
| @@ -141,6 +144,7 @@ public App create(@Valid @RequestBody AppModel appModel) { | |||
|
|
|||
| @PreAuthorize(value = "@permissionValidator.isAppAdmin(#appId)") | |||
| @PutMapping("/{appId:.+}") | |||
| @ApolloAuditLog(type = OpType.UPDATE, name = "user-command.app.update") | |||
There was a problem hiding this comment.
| @ApolloAuditLog(type = OpType.UPDATE, name = "user-command.app.update") | |
| @ApolloAuditLog(type = OpType.UPDATE, name = "app.update") |
if in openapi, use openapi.app.update
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
| ) | ||
| return d.promise | ||
| } | ||
| } |
| d.reject(result); | ||
| } | ||
| ) | ||
| return d.promise |
| }, function (result) { | ||
| d.reject(result); | ||
| } | ||
| ) |
| d.reject(result); | ||
| } | ||
| ) | ||
| return d.promise |
| }, function (result) { | ||
| d.reject(result); | ||
| } | ||
| ) |
| d.reject(result); | ||
| } | ||
| ) | ||
| return d.promise |
| }, function (result) { | ||
| d.reject(result); | ||
| } | ||
| ) |
| d.reject(result); | ||
| } | ||
| ) | ||
| return d.promise |
| }, function (result) { | ||
| d.reject(result); | ||
| } | ||
| ) |
| } | ||
| $scope.relatedDataInfluences = $scope.relatedDataInfluences.concat(result); | ||
|
|
||
| }) |
| $scope.showingDetail = {}; | ||
| $scope.relatedDataInfluences = []; | ||
| $scope.relatedDataInfluencePage = 0; | ||
| let RelatedDataInfluencePageSize = 10; |
There was a problem hiding this comment.
'let' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
| return d.promise | ||
| } | ||
| } | ||
| }]) |
BlackBear2003
force-pushed
the
add-audit-logs
branch
from
c43217e to
5e49df6
Compare
What's the purpose of this PR
add audit log module, PR for code review and further discussion
Which issue(s) this PR fixes:
Fixes #
#3505
Brief changelog
Add Apollo-Audit module which contains 4 modules of annotation, api, impl, springbootstarter
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.