Skip to content

fix: use QJsonObject for event logger messages#187

Open
Ivy233 wants to merge 1 commit intolinuxdeepin:masterfrom
Ivy233:feat/event-logger-multifield
Open

fix: use QJsonObject for event logger messages#187
Ivy233 wants to merge 1 commit intolinuxdeepin:masterfrom
Ivy233:feat/event-logger-multifield

Conversation

@Ivy233
Copy link
Copy Markdown
Contributor

@Ivy233 Ivy233 commented Apr 26, 2026

Use QJsonObject directly for EventLoggerData messages so JSON payloads are preserved when writing event logs. Remove the string key/value helper that no longer matches the object-based message payload.

使用 QJsonObject 直接承载 EventLoggerData 的 message 字段,确保写入事件日志时保留 JSON 载荷内容。移除不再匹配对象化 message 载荷的字符串键值辅助接口。

Summary by Sourcery

Switch event logger message payloads to use QJsonObject instead of string maps to preserve structured JSON data.

Bug Fixes:

  • Ensure event logger messages retain full JSON payloads by storing them as QJsonObject rather than flattening to string key/value pairs.

Enhancements:

  • Remove the string-based writeEventLog helper and simplify JSON serialization by writing the QJsonObject message directly into the event log output.

Use QJsonObject directly for EventLoggerData messages so JSON payloads are preserved when writing event logs. Remove the string key/value
helper that no longer matches the object-based message payload.

使用 QJsonObject 直接承载 EventLoggerData 的 message 字段,确保写入事件日志时保留 JSON 载荷内容。移除不再匹配对象化 message
载荷的字符串键值辅助接口。
@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Ivy233

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sourcery-ai
Copy link
Copy Markdown

sourcery-ai Bot commented Apr 26, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Refactors EventLoggerData to store event messages as a QJsonObject instead of a string map so structured JSON payloads are preserved, and simplifies EventLogger by removing the now-mismatched string key/value helper and serializing the stored QJsonObject directly.

Class diagram for updated EventLoggerData and EventLogger

classDiagram
class EventLoggerData {
  qint64 tid
  QString target
  QJsonObject message
  EventLoggerData()
  EventLoggerData(qint64 tid, const QString &target, const QJsonObject &message)
}

class EventLogger {
  void writeEventLog(const EventLoggerData &data)
  bool init(QString package_id, bool enable_sig)
  QJsonDocument toJson(EventLoggerData data)
}

EventLogger --> EventLoggerData : uses
Loading

File-Level Changes

Change Details Files
Change EventLoggerData message storage from a string map to QJsonObject and propagate this through constructors and JSON serialization.
  • Replace the message field type from QMap<QString, QString> to QJsonObject in EventLoggerData.
  • Update the default constructor to initialize message with an empty QJsonObject instead of an empty QMap.
  • Update the value constructor to take a const QJsonObject & for message instead of a QMap<QString, QString>.
  • Update makeJsonDocument to assign data.message directly into the "message" JSON field instead of constructing it from a key/value iterator.
cpp-include/eventlogger.hpp
Remove the convenience overload that wrote event logs from a single key/value string pair, which no longer fits the object-based message payload.
  • Delete writeEventLog overload taking tid, target, key, and value and delegating to EventLoggerData with a single-entry map.
cpp-include/eventlogger.hpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

  • If simple key/value logging is still a common case, consider keeping a convenience writeEventLog(tid, target, key, value) that internally constructs a QJsonObject so existing call sites don’t all have to manually build JSON objects.
  • Since EventLoggerData::message changed type, double-check any implicit uses (e.g., brace-initialization with { {"key", "value"} }) still behave as intended with QJsonObject, or consider adding explicit factory/helpers to make the new usage pattern clearer.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- If simple key/value logging is still a common case, consider keeping a convenience `writeEventLog(tid, target, key, value)` that internally constructs a `QJsonObject` so existing call sites don’t all have to manually build JSON objects.
- Since `EventLoggerData::message` changed type, double-check any implicit uses (e.g., brace-initialization with `{ {"key", "value"} }`) still behave as intended with `QJsonObject`, or consider adding explicit factory/helpers to make the new usage pattern clearer.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

这段代码的修改主要是将 EventLoggerData 结构体中的 message 成员从 QMap<QString, QString> 类型改为 QJsonObject 类型。这是一个非常好的重构方向。以下是对该 diff 的详细审查和改进意见:

1. 语法与逻辑审查

  • 通过:代码逻辑清晰,类型替换正确。构造函数的初始化列表和参数列表都已正确更新为 QJsonObject
  • 通过:删除了 writeEventLog 的一个重载版本(接受 keyvalue 的版本),这符合简化逻辑的趋势,因为调用方现在可以直接构造 QJsonObject 传入。
  • 通过formatEventLog 方法中,原本通过 QMapIterator 遍历 message 并手动构建 QJsonObject 的逻辑被替换为直接赋值 jsonObject["message"] = data.message;。这不仅消除了冗余代码,还利用了 QJsonObject 的隐式共享特性,提高了效率。

2. 代码质量改进意见

  • 建议使用 using 代替 typedef
    在 C++11 及更高版本中,推荐使用 using 别名声明来代替 typedef,因为它更易读,且支持模板别名。

    // 修改前
    typedef struct _EventLoggerData { ... } EventLoggerData;
    
    // 修改后
    using EventLoggerData = struct _EventLoggerData { ... };

    或者直接去掉 struct 前缀(如果这是 C++ 代码而非 C 兼容代码):

    struct EventLoggerData { ... };
  • 建议移除冗余的默认初始化
    _EventLoggerData 的默认构造函数中:

    _EventLoggerData()
        : tid(0)
        , target(QString()) // 冗余
        , message(QJsonObject()) // 冗余
    {}

    QStringQJsonObject 的默认构造函数已经将它们初始化为空对象/空字符串。显式调用 QString()QJsonObject() 是多余的。可以简化为:

    _EventLoggerData()
        : tid(0) // tid 是基本类型,需要显式初始化为0
    {}

    或者使用 = default(如果不需要特定逻辑):

    _EventLoggerData() = default;

    但要注意,= default 不会将 tid 初始化为 0(除非是类内初始化),所以如果 tid 必须为 0,保留初始化列表或使用类内成员初始化更好:

    qint64 tid = 0;

3. 代码性能改进意见

  • 关于参数传递的优化
    在构造函数 _EventLoggerData(qint64 tid, const QString &target, const QJsonObject &message) 中,参数传递使用了 const T&
    • 对于 QStringQJsonObject,Qt 的容器类使用了隐式共享(Copy-on-Write)。传递 const QString& 是标准做法。
    • 然而,在 C++11 之后,对于会被拷贝到类成员中的参数,使用值传递结合 std::move 往往更高效,特别是当传入的参数是右值(临时对象)时。
    • 改进建议
      // 按值传递,然后在初始化列表中使用 std::move
      _EventLoggerData(qint64 tid, QString target, QJsonObject message)
          : tid(tid)
          , target(std::move(target))       // 避免左值的深拷贝,直接转移资源
          , message(std::move(message))     // 避免左值的深拷贝,直接转移资源
      {}
      这样,如果调用方传入临时对象(如 writeEventLog(1, "app", QJsonObject{{"k", "v"}})),移动构造函数会被调用,性能更好。

4. 代码安全改进意见

  • 潜在的线程安全问题

    • QJsonObject可重入的,但非线程安全的。如果 EventLogger 是在多线程环境中使用(例如 tid 字段暗示了这一点),且不同的线程同时修改或读取同一个 EventLoggerData 实例的 message 成员,可能会导致数据竞争。
    • 审查:目前的 writeEventLog 接收的是 EventLoggerData副本void writeEventLog(EventLoggerData data)),这意味着在进入函数作用域后,操作的是局部对象。只要 m_writeEventLog 内部实现是线程安全的(例如使用了互斥锁),目前的实现是安全的。
    • 建议:确保 m_writeEventLog(这是一个回调函数,推测是写入文件或网络)的实现方已经处理了并发写入的问题。
  • 数据有效性检查
    formatEventLog 中,直接使用了 data.message。虽然 QJsonObject 处理空对象是安全的,但如果业务逻辑上要求 message 必须包含某些特定字段,建议在 writeEventLog 入口处或 formatEventLog 内部增加校验逻辑,防止记录无效的日志。

总结

这个 diff 是一个积极的改动,它简化了数据转换逻辑,利用了 JSON 原生对象减少了中间转换。

推荐修改后的代码片段示例(结合了上述建议):

#include <QJsonObject>
#include <QJsonDocument>
#include <QString>

// 使用 using 替代 typedef
using EventLoggerData = struct _EventLoggerData
{
    qint64 tid = 0; // 类内初始化,简化构造函数
    QString target;
    QJsonObject message;

    // 默认构造函数使用 = default
    _EventLoggerData() = default;

    // 参数按值传递,利用移动语义优化性能
    _EventLoggerData(qint64 tid, QString target, QJsonObject message)
        : tid(tid)
        , target(std::move(target))
        , message(std::move(message))
    {
    }
};

EventLogger 类中:

    // ... 其他代码 ...

    QJsonDocument formatEventLog(const EventLoggerData &data) const // 建议加上 const
    {
        QJsonObject jsonObject;
        jsonObject["tid"] = data.tid;
        jsonObject["target"] = data.target;
        
        // 直接赋值,利用隐式共享,非常高效
        jsonObject["message"] = data.message; 
        
        return QJsonDocument(jsonObject);
    }

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.

2 participants