[SERVER-70216] Replace 26 `Command` class-attribute getters with one struct and one getter Created: 04/Oct/22  Updated: 05/Sep/23

Status: Backlog
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Improvement Priority: Major - P3
Reporter: Billy Donahue Assignee: Backlog - Service Architecture
Resolution: Unresolved Votes: 0
Labels: neweng
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Assigned Teams:
Service Arch
Sprint: Service Arch 2023-09-04
Participants:

 Description   

There are now several virtual functions in Command that are giving hardcoded answers to per-type properties of the Command instance.

We seem to have been adding new ones at a steady pace for years.
For example, this function, though virtual, has nothing to do with the per-instance state of a Command object, and its value is determined only by the Command's dynamic type.

    /**
     * Return true if only the admin ns has privileges to run this command.
     */
    virtual bool adminOnly() const {
        return false;
    }

Most of these scattered hard-coded virtual functions would be far better off implemented as one big attributes structure with a single virtual accessor that returns a const ref to the per-class instance of that attributes structure. We really don't need to burn 7 lines of code and bloat the Command API for each little thing like this.

The Command class is now 300 lines long and it's mostly these properties.
These 26 (and counting!) virtual functions are:

    virtual std::size_t reserveBytesForReply() const {
    virtual bool adminOnly() const {
    virtual bool isHelloOrAuth() const {
    virtual const std::set<std::string>& apiVersions() const;
    virtual const std::set<std::string>& deprecatedApiVersions() const;
    virtual bool skipApiVersionCheck() const {
    virtual bool localHostOnlyIfNoAuth() const {
    virtual AllowedOnSecondary secondaryAllowed(ServiceContext* context) const = 0;
    virtual bool shouldAffectCommandCounter() const {
    virtual bool shouldAffectReadConcernCounter() const {
    virtual bool collectsResourceConsumptionMetrics() const {
    virtual bool requiresAuth() const {
    virtual std::string help() const {
    virtual std::set<StringData> sensitiveFieldNames() const {
    virtual bool maintenanceMode() const {
    virtual bool maintenanceOk() const {
    virtual LogicalOp getLogicalOp() const {
    virtual ReadWriteType getReadWriteType() const {
    virtual bool attachLogicalSessionsToOpCtx() const {
    virtual bool auditAuthorizationFailure() const {
    virtual bool allowedWithSecurityToken() const {
    virtual const AuthorizationContract* getAuthorizationContract() const {
    virtual bool supportsRetryableWrite() const {
    virtual bool shouldCheckoutSession() const {
    virtual bool isTransactionCommand() const {
    virtual bool allowedInTransactions() const {



 Comments   
Comment by Erin McNulty [ 30/Aug/23 ]

Looked at this on 8/29, and decided that it would require changing 250+ files and should be automated.

 

We could put in a PR that makes this change as an example on one command and then automate the rest of it, but leaving it for now.

Generated at Thu Feb 08 06:15:35 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.