FYI @eladiw

This pull request introduces 1 alert when merging 98de3ca into 2e8c1b6 - view on LGTM.com

new alerts:

• 1 for Useless assignment to local variable

This pull request introduces 2 alerts when merging c38223b into 6468d57 - view on LGTM.com

new alerts:

• 2 for Useless assignment to local variable

It looks like the link url in the bundle is leaving out the :iterate parameters:

GET https://localhost:44348/MedicationRequest?_include=MedicationRequest:patient&_include:iterate=Patient:general-practitioner&_id=123

{
  "resourceType": "Bundle",
  "id": "949805aa-9bc2-4195-b581-98df15005424",
  "meta": {
    "lastUpdated": "2020-10-09T20:29:01.8167293+00:00"
  },
  "type": "searchset",
  "link": [
    {
      "relation": "self",
      "url": "https://localhost:44348/MedicationRequest?_include=MedicationRequest%3Apatient&_id=123"
    }
  ]
}

It looks like the link url in the bundle is leaving out the :iterate parameters:

@johnstairs good catch!
It's since the include iterate parameters appear in the unsupportedSearchParameters due to issue: FirelyTeam/firely-net-sdk#222.

I removed them from the unsupported parameters list and added validation tests.

Parenthesis indentation is inconsistent (let's follow the style of cte7):

cte7 AS
(
    SELECT DISTINCT Sid1, IsMatch, 0 AS IsPartial
    FROM cte6
),
cte8 AS
(
    SELECT DISTINCT refTarget.ResourceSurrogateId AS Sid1, 0 AS IsMatch
    FROM dbo.ReferenceSearchParam refSource
    INNER JOIN dbo.Resource refTarget
    ON refSource.ReferenceResourceTypeId = refTarget.ResourceTypeId
        AND refSource.ReferenceResourceId = refTarget.ResourceId
    WHERE refSource.SearchParamId = @p11
        AND refTarget.IsHistory = 0
        AND refSource.IsHistory = 0
        AND refSource.ResourceTypeId = @p12
        AND refSource.ResourceSurrogateId IN (SELECT TOP(@p13) Sid1 FROM cte3)
    ),
cte9 AS
    (
    SELECT DISTINCT TOP (@p14) Sid1, IsMatch, CASE WHEN count(*) over() > @p15 THEN 1 ELSE 0 END AS IsPartial
    FROM cte8
    ),
cte10 AS
    (
    SELECT DISTINCT refTarget.ResourceSurrogateId AS Sid1, 0 AS IsMatch
    FROM dbo.ReferenceSearchParam refSource
    INNER JOIN dbo.Resource refTarget
    ON refSource.ReferenceResourceTypeId = refTarget.ResourceTypeId
        AND refSource.ReferenceResourceId = refTarget.ResourceId
    WHERE refSource.SearchParamId = @p11
        AND refTarget.IsHistory = 0
        AND refSource.IsHistory = 0
        AND refSource.ResourceTypeId = @p12
        AND refSource.ResourceSurrogateId IN (SELECT TOP(@p16) Sid1 FROM cte7)
),

Parenthesis indentation is inconsistent (let's follow the style of cte7):

cte7 AS
(
    SELECT DISTINCT Sid1, IsMatch, 0 AS IsPartial
    FROM cte6
),
cte8 AS
(
    SELECT DISTINCT refTarget.ResourceSurrogateId AS Sid1, 0 AS IsMatch
    FROM dbo.ReferenceSearchParam refSource
    INNER JOIN dbo.Resource refTarget
    ON refSource.ReferenceResourceTypeId = refTarget.ResourceTypeId
        AND refSource.ReferenceResourceId = refTarget.ResourceId
    WHERE refSource.SearchParamId = @p11
        AND refTarget.IsHistory = 0
        AND refSource.IsHistory = 0
        AND refSource.ResourceTypeId = @p12
        AND refSource.ResourceSurrogateId IN (SELECT TOP(@p13) Sid1 FROM cte3)
    ),
cte9 AS
    (
    SELECT DISTINCT TOP (@p14) Sid1, IsMatch, CASE WHEN count(*) over() > @p15 THEN 1 ELSE 0 END AS IsPartial
    FROM cte8
    ),
cte10 AS
    (
    SELECT DISTINCT refTarget.ResourceSurrogateId AS Sid1, 0 AS IsMatch
    FROM dbo.ReferenceSearchParam refSource
    INNER JOIN dbo.Resource refTarget
    ON refSource.ReferenceResourceTypeId = refTarget.ResourceTypeId
        AND refSource.ReferenceResourceId = refTarget.ResourceId
    WHERE refSource.SearchParamId = @p11
        AND refTarget.IsHistory = 0
        AND refSource.IsHistory = 0
        AND refSource.ResourceTypeId = @p12
        AND refSource.ResourceSurrogateId IN (SELECT TOP(@p16) Sid1 FROM cte7)
),

@johnstairs Yes, this is because SQLQueryGenerator uses Microsoft.Health.SqlServer.IndentedStringBuilder which has no option to un-indent (removing chars from the end doesn;t work and is quite ugly).
I don't see how we can work around it.

Note that when calling recursively to AcceptVisitor, in the case where we are iterating over multiple results sets, that is done within an already indented expression sue to the main call:

           StringBuilder.AppendDelimited($",{Environment.NewLine}", expression.TableExpressions, (sb, tableExpression) =>
            {
                sb.Append(TableExpressionName(++_tableExpressionCounter)).AppendLine(" AS").AppendLine("(");

                using (sb.Indent())
                {
                    tableExpression.AcceptVisitor(this, context);
                }

                sb.Append(")");
            });

In cte4 below, we apply a top clause on the resultset from cte3 we use for the intersection:

      cte3 AS
      (
          SELECT DISTINCT Sid1, IsMatch, 0 AS IsPartial
          FROM cte2
      ),
      cte4 AS
      (
          SELECT DISTINCT refTarget.ResourceSurrogateId AS Sid1, 0 AS IsMatch
          FROM dbo.ReferenceSearchParam refSource
          INNER JOIN dbo.Resource refTarget
          ON refSource.ReferenceResourceTypeId = refTarget.ResourceTypeId
              AND refSource.ReferenceResourceId = refTarget.ResourceId
          WHERE refSource.SearchParamId = @p5
              AND refTarget.IsHistory = 0
              AND refSource.IsHistory = 0
              AND refSource.ResourceTypeId = @p6
              AND refSource.ResourceSurrogateId IN (SELECT TOP(@p7) Sid1 FROM cte3)
      )

We should not only be applying iterate to the first ten includes in cte3. (We've also discussed that we should apply an overall limit to included resources like we do for _revinclude, though I'm happy to take that as a separate PR)

True, but I thought we agreed it will be in a separated PR. We even have an open issue for it: #1309

Regarding the indentation, let's revisit later. We can expose the indentation level as a property on the writer, but I think I would look at an approach that does not require recursion. But later...

It looks like we still are applying a TOP clause on the intersection for non-iterate includes:

      DECLARE @p0 SmallInt = 77;
      DECLARE @p1 VarChar(64) = '1';
      DECLARE @p2 Int = 11;
      DECLARE @p3 SmallInt = 828;
      DECLARE @p4 Int = 10;

      SET STATISTICS IO ON;
      SET STATISTICS TIME ON;

      WITH cte0 AS
      (
          SELECT ResourceSurrogateId AS Sid1
          FROM dbo.Resource
          WHERE IsHistory = 0
              AND IsDeleted = 0
              AND ResourceTypeId = @p0
              AND ResourceId = @p1

      ),
      cte1 AS
      (
          SELECT DISTINCT TOP (@p2) Sid1, 1 AS IsMatch, 0 AS IsPartial
          FROM cte0
          ORDER BY  Sid1 ASC
      ),
      cte2 AS
      (
          SELECT DISTINCT refTarget.ResourceSurrogateId AS Sid1, 0 AS IsMatch
          FROM dbo.ReferenceSearchParam refSource
          INNER JOIN dbo.Resource refTarget
          ON refSource.ReferenceResourceTypeId = refTarget.ResourceTypeId
              AND refSource.ReferenceResourceId = refTarget.ResourceId
          WHERE refSource.SearchParamId = @p3
              AND refTarget.IsHistory = 0
              AND refSource.IsHistory = 0
              AND refSource.ResourceTypeId = @p0
              AND refSource.ResourceSurrogateId IN (SELECT TOP(@p4) Sid1 FROM cte1)
      ),
      cte3 AS
      (
          SELECT DISTINCT Sid1, IsMatch, 0 AS IsPartial
          FROM cte2
      ),
...

I don't know if that's new from this PR...

It looks like we still are applying a TOP clause on the intersection for non-iterate includes:

      DECLARE @p0 SmallInt = 77;
      DECLARE @p1 VarChar(64) = '1';
      DECLARE @p2 Int = 11;
      DECLARE @p3 SmallInt = 828;
      DECLARE @p4 Int = 10;

      SET STATISTICS IO ON;
      SET STATISTICS TIME ON;

      WITH cte0 AS
      (
          SELECT ResourceSurrogateId AS Sid1
          FROM dbo.Resource
          WHERE IsHistory = 0
              AND IsDeleted = 0
              AND ResourceTypeId = @p0
              AND ResourceId = @p1

      ),
      cte1 AS
      (
          SELECT DISTINCT TOP (@p2) Sid1, 1 AS IsMatch, 0 AS IsPartial
          FROM cte0
          ORDER BY  Sid1 ASC
      ),
      cte2 AS
      (
          SELECT DISTINCT refTarget.ResourceSurrogateId AS Sid1, 0 AS IsMatch
          FROM dbo.ReferenceSearchParam refSource
          INNER JOIN dbo.Resource refTarget
          ON refSource.ReferenceResourceTypeId = refTarget.ResourceTypeId
              AND refSource.ReferenceResourceId = refTarget.ResourceId
          WHERE refSource.SearchParamId = @p3
              AND refTarget.IsHistory = 0
              AND refSource.IsHistory = 0
              AND refSource.ResourceTypeId = @p0
              AND refSource.ResourceSurrogateId IN (SELECT TOP(@p4) Sid1 FROM cte1)
      ),
      cte3 AS
      (
          SELECT DISTINCT Sid1, IsMatch, 0 AS IsPartial
          FROM cte2
      ),
...

I don't know if that's new from this PR...

It wasn't introduced by this PR. I only removed the TOP for iterate expressions since they select from an IncludeLimit cte.

I think we should change the non-iterate Top in a separate PR. The include limit feature is not tested AFAIK and there's a bit of a challenge to test it by providing a test configuration. It deserves attention.
I think it should be part of this issue: #1309

/azp run

@johnstairs The build seems to fail on issues not related to the PR changes, but rather environment setup.
For example, provisioning environment, while a previous environment exists:

@limorl Sorry, your PR environment got stuck in a bad state due to a provisioning issue we were having last week. I'm cleaning up the environment and will kick off a new build.

/azp run