graphql-dotnet: wrong depth calculation with fragments

it’s exceedling clear that MaxDepth should refer to the maximum depth (whether counted as AST nodes or otherwise) of the deepest AST node, counting through selected fragments.

Nope, no deepest, it’s sum of all depths. Each field (as is or from fragment) counts - TotalQueryDepth incremented by 1 every time the ComplexityVisitor visits field.

‘total nodes’ makes sense, and ‘maximum depth’ makes sense, but I have no idea what ‘total depth’ would refer to.

Just a problem of naming. I agree that ‘total nodes’ sounds more clear. Most likely Initial author considered the number of nodes in the context of the depth so… We can make naming even simpler:

public class ComplexityResult
{
        public Dictionary<ASTNode, double> ComplexityMap { get; } = new Dictionary<ASTNode, double>();

        public double Complexity { get; set; }

        public int NodesCount { get; set; } <--- current TotalQueryDepth

        public int MaxDepth { get; set; } <---- new property, maximum depth of the deepest AST node, finally!
}

but it seems to me that a complexity calculation would also involve assigning a cost to each node

#298 / #1254

My intuition tells me that this task should be postponed, because ComplexityAnalyzer can be changed a lot. Even if we rename properties now, it will not change anything fundamentally, and the comments can be added after the v4 release. I am not familiar with ComplexityAnalyzer so that with 100% confidence to write comments and documentation. I do not want to delay the v4 release because of this so I removed this task from 4.0 milestone.

I’m having the same (or a similar) issue - the calculated depth increases as I add more fields even though there are no recursive relationships and the apparent depth remains constant.

Did this ever get resolved? Or am I just misunderstanding what “depth” means in this context?

The following returns a depth of 38. (count and year are integers)

query{
  conditionsPerYear{
    filing {
      REQUIRED {
        count
        year
      }
      NOT_REQUIRED {
        count
        year
      }
    }
    status {
      CLOSED {
        count
        year
      }
      IN_PROGRESS {
        count
        year
      }
    }
    theme {
      ADMINISTRATIVE_ {
        count
        year
      }
			CONSERVATION_OF_RESOURCES {
        count
        year
      }
      CROSSINGS {
        count
        year
      }
      DAMAGE_PREVENTION {
        count
        year
      }
      ENFORCEMENT {
        count
        year
      }
      ENGINEERING {
        count
        year
      }
      ENVIRONMENTAL_PROTECTION {
        count
        year
      }
      FINANCIAL {
        count
        year
      }
      IAMC_OBSERVATION {
        count
        year
      }
      INTEGRITY_MANAGEMENT {
        count
        year
      }
      LANDOWNER {
        count
        year
      }
      MANAGEMENT_SYSTEM {
        count
        year
      }
      PUBLIC_AWARENESS {
        count
        year
      }
      SAFETY_MANAGEMENT {
        count
        year
      }
      SECURITY {
        count
        year
      }
      SOCIOECONOMIC {
        count
        year
      }
      STANDARD_CONDITION {
        count
        year
      }
      SUNSET_CLAUSE {
        count
        year
      }
    }
    timing {
      ABANDONMENT {
        count
        year
      }
      DURING_CONSTRUCTION_PHASE {
        count
        year
      }
      EXPIRY_DATE_OF_REG_INSTR {
        count
        year
      }
      INCLUDES_ALL_PHASES_OF_CONSTR {
        count
        year
      }
      NOT_CONSTRUCTION_RELATED {
        count
        year
      }
      POST_CONSTRUCTION_PHASE {
        count
        year
      }
      PRIOR_TO_CONSTRUCTION_PHASE {
        count
        year
      }
      UNSPECIFIED {
        count
        year
      }
    }
    type {
      NON_STANDARD {
        count
        year
      }
      STANDARD {
        count
        year
      }
      
    }
  }
}

is max depth still calculates total depth?

Yes. This is precisely the sum of all the depths in terms of AST, and not the maximum depth of the most nested branch of AST. I rewrote complexity analizer in #2863 . Honestly, I forgot about this issue. Regarding my https://github.com/graphql-dotnet/graphql-dotnet/issues/344#issuecomment-797060698 - now I better understand all this stuff to make proper changes in comments or/and rename properties. Total word may be confusing. I mean both complexity and depth terms work as total numbers (see three_depth_query_wide test) but only one (TotalQueryDepth) has total in its name. I think that I would remove Total word from TotalQueryDepth property at all (and I feel that Query word may be removed as well).

So I suggest this:

public class ComplexityResult
{
        public Dictionary<ASTNode, double> ComplexityMap { get; } = new Dictionary<ASTNode, double>();

        public double Complexity { get; set; }

        public int Depth { get; set; }
}

At the same time Total meaning should be described in xml comments for both Complexity and Depth properties since they both accumulate total metrics. Also ping @Shane32 for comments.

I’m in the same boat.

The xml comments are extremely basic; indeed I wrote them myself, just so there was more than nothing. But I have not used the complexity analyzer, and I don’t fully understand what the properties mean.

I suggest we add significantly more text to the xml comment, and then assess if the properties need to be renamed and what would be proper.

At first glance, I would think the names in the configuration should match the names in the response, so, for example only, MaxTotalDepth and TotalDepth. Or MaxDepth and Depth. Or MaxTotalQueryDepth and TotalQueryDepth. The xml comment should make it very clear how these numbers are calculated.