[I] [Enhancement] Extract user-property validation in SendMessageActivity#buildMessageProperty into an overridable checkUserProperties method [rocketmq]

["ymwneu (via GitHub)" <gi...@apache.org>]

ymwneu opened a new issue, #11000:
URL: https://github.com/apache/rocketmq/issues/11000

   ### Before Creating the Enhancement Request
   
   - [x] I have confirmed that this should be classified as an enhancement rather than a bug/feature.
   
   
   ### Summary
   
   SendMessageActivity#buildMessageProperty() (grpc v2 producer) inlines the validation of user properties — max property count, reserved system-property keys, and control-character checks — directly inside a large method. This proposal extracts that validation block into a small, standalone protected void checkUserProperties(Map<String, String> userProperties) method that buildMessageProperty() calls, so it can be overridden independently.
   
   ### Motivation
   
   Some deployments need custom rules for user message properties — for example, allowing certain properties to contain control characters in specific business scenarios, while keeping every other property/message-building rule untouched.
   
   Today the only extension point is overriding the entire buildMessageProperty() method, which contains a large amount of unrelated logic (tag/keys handling, transaction properties, delay/deliver-timestamp handling, priority, message group, lite topic, trace context, born host/timestamp, etc.). Overriding it just to tweak one validation rule means:
     - Duplicating a large method that isn't the actual customization target.
     - High maintenance cost: any upstream change to buildMessageProperty() requires manually re-syncing the downstream override, risking divergence or missed fixes.
   
     Extracting the check logic into its own protected method gives a narrow, stable extension seam for this kind of customization without touching the rest of the message-building logic.
   
   ### Describe the Solution You'd Like
   
   Add a new protected method in SendMessageActivity:
   
     protected void checkUserProperties(Map<String, String> userProperties) {
         ProxyConfig config = ConfigurationManager.getProxyConfig();
         if (userProperties.size() > config.getUserPropertyMaxNum()) {
             throw new GrpcProxyException(Code.MESSAGE_PROPERTIES_TOO_LARGE, "too many user properties, max is " + config.getUserPropertyMaxNum());
         }
         for (Map.Entry<String, String> userPropertiesEntry : userProperties.entrySet()) {
             if (MessageConst.STRING_HASH_SET.contains(userPropertiesEntry.getKey())) {
                 throw new GrpcProxyException(Code.ILLEGAL_MESSAGE_PROPERTY_KEY, "property is used by system: " + userPropertiesEntry.getKey());
             }
             if (GrpcValidator.getInstance().containControlCharacter(userPropertiesEntry.getKey())) {
                 throw new GrpcProxyException(Code.ILLEGAL_MESSAGE_PROPERTY_KEY, "the key of property cannot contain control character");
             }
             if (GrpcValidator.getInstance().containControlCharacter(userPropertiesEntry.getValue())) {
                 throw new GrpcProxyException(Code.ILLEGAL_MESSAGE_PROPERTY_KEY, "the value of property cannot contain control character");
             }
         }
     }
   
     buildMessageProperty() calls checkUserProperties(userProperties) and keeps the property-size accumulation (used later for the total-size check) in place, unchanged. No behavior change for existing callers — this is a pure refactor/extension-point addition, verified by the existing SendMessageActivityTest suite, which passes unmodified.
   
   ### Describe Alternatives You've Considered
   
   - Override buildMessageProperty() entirely — works today, but forces re-implementing/copy-pasting large amounts of unrelated logic and creates ongoing merge/maintenance burden as upstream evolves.
     - Add a config switch/flag (e.g. proxyConfig.isAllowControlCharacterInUserProperty()) to toggle the control-character check — simpler for this one case, but less flexible for other custom validation rules downstream teams may want, and adds proxy-config surface for a narrow use case. An overridable method is more general and doesn't require upstream to anticipate every possible customization.
     - Introduce a pluggable validator interface/SPI for message properties — more flexible but a larger design change; extracting the method first is a minimal, low-risk step that doesn't preclude a future SPI-based approach.
   
   ### Additional Context
   
   _No response_


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [Enhancement] Extract user-property validation in SendMessageActivity#buildMessageProperty into an overridable checkUserProperties method [rocketmq]

["RockteMQ-AI (via GitHub)" <gi...@apache.org>]

RockteMQ-AI commented on issue #11000:
URL: https://github.com/apache/rocketmq/issues/11000#issuecomment-5522594344

   **Issue Evaluation**
   
   Category: `type/enhancement` | Status: **Evaluated**
   
   **Feasibility:** Feasible
   **Scope:** Proxy module (`SendMessageActivity` in gRPC v2 producer path)
   **Compatibility:** No breaking changes — purely internal refactoring.
   
   This is a well-motivated refactoring request. Extracting the user-property validation logic from `buildMessageProperty()` into a dedicated `protected void checkUserProperties(Map<String, String>)` method provides a narrow, stable extension point for deployments that need custom property validation rules.
   
   The current design forces downstream users to override the entire `buildMessageProperty()` method (which handles tag/keys, transaction properties, delay/deliver-timestamp, priority, message group, lite topic, trace context, etc.) just to customize one validation block. This creates unnecessary maintenance burden and divergence risk.
   
   **Recommendation:** Accept. The change is low-risk, improves extensibility, and follows the Open/Closed Principle.
   
   ---
   *Automated evaluation by RockteMQ-AI*


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] [Enhancement] Extract user-property validation in SendMessageActivity#buildMessageProperty into an overridable checkUserProperties method [rocketmq]

["lizhimins (via GitHub)" <gi...@apache.org>]

lizhimins closed issue #11000: [Enhancement] Extract user-property validation in SendMessageActivity#buildMessageProperty into an overridable checkUserProperties method
URL: https://github.com/apache/rocketmq/issues/11000


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org