Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 2780 #2785

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1025,7 +1025,7 @@ internal static ConnectorType GetCdpTableType(ICdpTableResolver tableResolver, s
IList<ReferencedEntity> referencedEntities = GetReferenceEntities(connectorName, stringValue);

SymbolTable symbolTable = new SymbolTable();
ConnectorType connectorType = new ConnectorType(jsonElement, tableName, symbolTable, compatibility, referencedEntities, datasetName, name, connectorName, tableResolver, serviceCapabilities, isTableReadOnly);
ConnectorType connectorType = new ConnectorType(jsonElement, tableName, symbolTable, compatibility, referencedEntities, datasetName, name, tableResolver, serviceCapabilities, isTableReadOnly);
delegationInfo = ((DataSourceInfo)connectorType.FormulaType._type.AssociatedDataSources.First()).DelegationInfo;
optionSets = symbolTable.OptionSets.Select(kvp => kvp.Value);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ public class PowerPlatformConnectorClient : HttpClient

public string EnvironmentId { get; set; }

public string RequestUrlPrefix { get; }
Copy link
Contributor

@MikeStall MikeStall Dec 18, 2024

Choose a reason for hiding this comment

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

RequestUrlPrefix

consider making this an Init property, and then we don't need the extra ctors. #Resolved


/// <summary>
/// Initializes a new instance of the <see cref="PowerPlatformConnectorClient"/> class.
/// </summary>
Expand All @@ -69,6 +71,20 @@ public PowerPlatformConnectorClient(string endpoint, string environmentId, strin
{
}

/// <summary>
/// Initializes a new instance of the <see cref="PowerPlatformConnectorClient"/> class.
/// </summary>
/// <param name="endpoint">APIM Endpoint.</param>
/// <param name="environmentId">Environment Id.</param>
/// /// <param name="requestUrlPrefix">Url Prefix for CDP connectors.</param>
/// <param name="connectionId">Connection/connector Id.</param>
/// <param name="getAuthToken">Function returning the JWT token.</param>
/// <param name="httpInvoker">Optional HttpMessageInvoker. If not provided a default HttpClient is used.</param>
public PowerPlatformConnectorClient(string endpoint, string environmentId, string requestUrlPrefix, string connectionId, Func<string> getAuthToken, HttpMessageInvoker httpInvoker = null)
: this(endpoint, environmentId, requestUrlPrefix, connectionId, getAuthToken, null, httpInvoker)
{
}

/// <summary>
/// Initializes a new instance of the <see cref="PowerPlatformConnectorClient"/> class.
/// </summary>
Expand All @@ -82,6 +98,20 @@ public PowerPlatformConnectorClient(string endpoint, string environmentId, strin
{
}

/// <summary>
/// Initializes a new instance of the <see cref="PowerPlatformConnectorClient"/> class.
/// </summary>
/// <param name="endpoint">APIM Endpoint.</param>
/// <param name="environmentId">Environment Id.</param>
/// <param name="requestUrlPrefix">Url Prefix for CDP connectors.</param>
/// <param name="connectionId">Connection/connector Id.</param>
/// <param name="getAuthToken">Async function returning the JWT token.</param>
/// <param name="httpInvoker">Optional HttpMessageInvoker. If not provided a default HttpClient is used.</param>
public PowerPlatformConnectorClient(string endpoint, string environmentId, string requestUrlPrefix, string connectionId, Func<Task<string>> getAuthToken, HttpMessageInvoker httpInvoker = null)
: this(endpoint, environmentId, requestUrlPrefix, connectionId, getAuthToken, null, httpInvoker)
{
}

/// <summary>
/// Initializes a new instance of the <see cref="PowerPlatformConnectorClient"/> class.
/// </summary>
Expand All @@ -103,13 +133,28 @@ public PowerPlatformConnectorClient(OpenApiDocument swaggerFile, string environm
/// <param name="environmentId">Environment Id.</param>
/// <param name="connectionId">Connection/connector Id.</param>
/// <param name="getAuthToken">Function returning the JWT token.</param>
/// /// <param name="userAgent">Product UserAgent to add to Power-Fx one (Power-Fx/version).</param>
/// <param name="userAgent">Product UserAgent to add to Power-Fx one (Power-Fx/version).</param>
/// <param name="httpInvoker">Optional HttpMessageInvoker. If not provided a default HttpClient is used.</param>
public PowerPlatformConnectorClient(string endpoint, string environmentId, string connectionId, Func<string> getAuthToken, string userAgent, HttpMessageInvoker httpInvoker = null)
: this(endpoint, environmentId, connectionId, async () => getAuthToken(), userAgent, httpInvoker)
{
}

/// <summary>
/// Initializes a new instance of the <see cref="PowerPlatformConnectorClient"/> class.
/// </summary>
/// <param name="endpoint">APIM Endpoint.</param>
/// <param name="environmentId">Environment Id.</param>
/// <param name="requestUrlPrefix">Url Prefix for CDP connectors.</param>
/// <param name="connectionId">Connection/connector Id.</param>
/// <param name="getAuthToken">Async function returning the JWT token.</param>
/// <param name="userAgent">Product UserAgent to add to Power-Fx one (Power-Fx/version).</param>
/// <param name="httpInvoker">Optional HttpMessageInvoker. If not provided a default HttpClient is used.</param>
public PowerPlatformConnectorClient(string endpoint, string environmentId, string requestUrlPrefix, string connectionId, Func<string> getAuthToken, string userAgent, HttpMessageInvoker httpInvoker = null)
: this(endpoint, environmentId, requestUrlPrefix, connectionId, async () => getAuthToken(), userAgent, httpInvoker)
{
}

/// <summary>
/// Initializes a new instance of the <see cref="PowerPlatformConnectorClient"/> class.
/// </summary>
Expand All @@ -120,13 +165,29 @@ public PowerPlatformConnectorClient(string endpoint, string environmentId, strin
/// <param name="userAgent">Product UserAgent to add to Power-Fx one (Power-Fx/version).</param>
/// <param name="httpInvoker">Optional HttpMessageInvoker. If not provided a default HttpClient is used.</param>
public PowerPlatformConnectorClient(string endpoint, string environmentId, string connectionId, Func<Task<string>> getAuthToken, string userAgent, HttpMessageInvoker httpInvoker = null)
: this(endpoint, environmentId, null, connectionId, getAuthToken, userAgent, httpInvoker)
{
}

/// <summary>
/// Initializes a new instance of the <see cref="PowerPlatformConnectorClient"/> class.
/// </summary>
/// <param name="endpoint">APIM Endpoint.</param>
/// <param name="environmentId">Environment Id.</param>
/// <param name="requestUrlPrefix">Url Prefix for CDP connectors.</param>
/// <param name="connectionId">Connection/connector Id.</param>
/// <param name="getAuthToken">Async function returning the JWT token.</param>
/// <param name="userAgent">Product UserAgent to add to Power-Fx one (Power-Fx/version).</param>
/// <param name="httpInvoker">Optional HttpMessageInvoker. If not provided a default HttpClient is used.</param>
public PowerPlatformConnectorClient(string endpoint, string environmentId, string requestUrlPrefix, string connectionId, Func<Task<string>> getAuthToken, string userAgent, HttpMessageInvoker httpInvoker = null)
{
_client = httpInvoker ?? new HttpClient();

GetAuthToken = getAuthToken ?? throw new ArgumentNullException(nameof(getAuthToken));
ConnectionId = connectionId ?? throw new ArgumentNullException(nameof(connectionId));
EnvironmentId = environmentId ?? throw new ArgumentNullException(nameof(environmentId));
UserAgent = string.IsNullOrWhiteSpace(userAgent) ? $"PowerFx/{Version}" : $"{userAgent} PowerFx/{Version}";
RequestUrlPrefix = requestUrlPrefix;

// Case insensitive comparison per RFC 9110 [4.2.3 http(s) Normalization and Comparison]
if (endpoint.StartsWith($"{Uri.UriSchemeHttp}://", StringComparison.OrdinalIgnoreCase))
Expand Down Expand Up @@ -171,7 +232,7 @@ public override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage req

public async Task<HttpRequestMessage> Transform(HttpRequestMessage request)
{
var url = request.RequestUri.OriginalString;
var url = $"{RequestUrlPrefix ?? string.Empty}{request.RequestUri.OriginalString}";
if (request.RequestUri.IsAbsoluteUri)
{
// Client has Basepath set.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ internal ConnectorType(ISwaggerSchema schema, ConnectorCompatibility compatibili
}

// Called by ConnectorFunction.GetCdpTableType
internal ConnectorType(JsonElement schema, string tableName, SymbolTable optionSets, ConnectorCompatibility compatibility, IList<ReferencedEntity> referencedEntities, string datasetName, string name, string connectorName, ICdpTableResolver resolver, ServiceCapabilities serviceCapabilities, bool isTableReadOnly)
internal ConnectorType(JsonElement schema, string tableName, SymbolTable optionSets, ConnectorCompatibility compatibility, IList<ReferencedEntity> referencedEntities, string datasetName, string name, ICdpTableResolver resolver, ServiceCapabilities serviceCapabilities, bool isTableReadOnly)
: this(SwaggerJsonSchema.New(schema), null, new SwaggerParameter(null, true, SwaggerJsonSchema.New(schema), null).GetConnectorType(tableName, optionSets, compatibility))
{
Name = name;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,21 @@ internal class CdpTableResolver : ICdpTableResolver

private readonly HttpClient _httpClient;

private readonly string _uriPrefix;
[Obsolete]
private readonly string _uriPrefix = null;
Copy link
Contributor

@MikeStall MikeStall Dec 18, 2024

Choose a reason for hiding this comment

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

_uriPrefix

can we remove and use httpClient? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so
This one is only used for backward compatibility


private readonly bool _doubleEncoding;

public CdpTableResolver(CdpTable tabularTable, HttpClient httpClient, bool doubleEncoding, ConnectorLogger logger = null)
{
_tabularTable = tabularTable;
_httpClient = httpClient;
_doubleEncoding = doubleEncoding;

Logger = logger;
}

[Obsolete]
public CdpTableResolver(CdpTable tabularTable, HttpClient httpClient, string uriPrefix, bool doubleEncoding, ConnectorLogger logger = null)
{
_tabularTable = tabularTable;
Expand All @@ -53,7 +64,12 @@ public async Task<ConnectorType> ResolveTableAsync(string tableName, Cancellatio
}

string dataset = _doubleEncoding ? CdpServiceBase.DoubleEncode(_tabularTable.DatasetName) : _tabularTable.DatasetName;
string uri = (_uriPrefix ?? string.Empty) + (UseV2(_uriPrefix) ? "/v2" : string.Empty) + $"/$metadata.json/datasets/{dataset}/tables/{CdpServiceBase.DoubleEncode(tableName)}?api-version=2015-09-01";

#pragma warning disable CS0612 // Type or member is obsolete
string prefix = string.IsNullOrEmpty(_uriPrefix) ? string.Empty : (_uriPrefix ?? string.Empty) + (UseV2(_uriPrefix) ? "/v2" : string.Empty);
#pragma warning restore CS0612 // Type or member is obsolete

string uri = $"{prefix}/$metadata.json/datasets/{dataset}/tables/{CdpServiceBase.DoubleEncode(tableName)}?api-version=2015-09-01";

string text = await CdpServiceBase.GetObject(_httpClient, $"Get table metadata", uri, null, cancellationToken, Logger).ConfigureAwait(false);

Expand Down Expand Up @@ -92,7 +108,10 @@ public async Task<ConnectorType> ResolveTableAsync(string tableName, Cancellatio
// sqlRelationships = GetSqlRelationships(text2);
//}

var parts = _uriPrefix.Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries);
#pragma warning disable CS0612 // Type or member is obsolete
var parts = string.IsNullOrEmpty(_uriPrefix) ? Array.Empty<string>() : _uriPrefix.Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries);
#pragma warning restore CS0612 // Type or member is obsolete

string connectorName = (parts.Length > 1) ? parts[1] : string.Empty;

ConnectorType connectorType = ConnectorFunction.GetCdpTableType(this, connectorName, _tabularTable.TableName, "Schema/Items", FormulaValue.New(text), ConnectorCompatibility.CdpCompatibility, _tabularTable.DatasetName,
Expand All @@ -103,8 +122,7 @@ public async Task<ConnectorType> ResolveTableAsync(string tableName, Cancellatio
return connectorType;
}

internal static bool IsSql(string uriPrefix) => uriPrefix.Contains("/sql/");

[Obsolete]
internal static bool UseV2(string uriPrefix) => uriPrefix.Contains("/sql/") ||
uriPrefix.Contains("/zendesk/");
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is ultimately used to determine the endpoint, can we remove from here and push higher?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure where you'd put it
This is used twice in this class, twice in CdpDataSource and once in CdpTable, always in [Obsolete] methods

If we want to absorb the breaking change fully we can jsut delete all these [Obsolete] things
Just let know if is you agree/want that?


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,13 @@ public CdpDataSource(string dataset)
DatasetName = dataset ?? throw new ArgumentNullException(nameof(dataset));
}

public static async Task<DatasetMetadata> GetDatasetsMetadataAsync(HttpClient httpClient, CancellationToken cancellationToken, ConnectorLogger logger = null)
{
string uri = $"/$metadata.json/datasets";
return await GetObject<DatasetMetadata>(httpClient, "Get datasets metadata", uri, null, cancellationToken, logger).ConfigureAwait(false);
}

[Obsolete("Use GetDatasetsMetadataAsync without urlPrefix")]
public static async Task<DatasetMetadata> GetDatasetsMetadataAsync(HttpClient httpClient, string uriPrefix, CancellationToken cancellationToken, ConnectorLogger logger = null)
{
string uri = (uriPrefix ?? string.Empty)
Expand All @@ -32,6 +39,21 @@ public static async Task<DatasetMetadata> GetDatasetsMetadataAsync(HttpClient ht
return await GetObject<DatasetMetadata>(httpClient, "Get datasets metadata", uri, null, cancellationToken, logger).ConfigureAwait(false);
}

public virtual async Task<IEnumerable<CdpTable>> GetTablesAsync(HttpClient httpClient, CancellationToken cancellationToken, ConnectorLogger logger = null)
{
if (DatasetMetadata == null)
{
DatasetMetadata = await GetDatasetsMetadataAsync(httpClient, cancellationToken, logger).ConfigureAwait(false);
}

string queryName = httpClient is PowerPlatformConnectorClient ppcc && ppcc.RequestUrlPrefix.Contains("/sharepointonline/") ? "/alltables" : "/tables";
Copy link
Contributor

@MikeStall MikeStall Dec 18, 2024

Choose a reason for hiding this comment

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

httpClient is PowerPlatformConnectorClient ppcc && ppcc.RequestUrlPrefix.Contains

The issue description in #2780 suggested dealing with this case by looking at HttpClient.BaseAddress - would that work? It would let us avoid the type cast.

nit- either way - can you encapsulate this in a tiny helper method IsSharepoint(httpClient)?
#Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not in the HttpClient base address unfortunately
I will encapsulate in IsSharepoint()

string uri = $"/datasets/{(DatasetMetadata.IsDoubleEncoding ? DoubleEncode(DatasetName) : DatasetName)}" + queryName;

GetTables tables = await GetObject<GetTables>(httpClient, "Get tables", uri, null, cancellationToken, logger).ConfigureAwait(false);
return tables?.Value?.Select((RawTable rawTable) => new CdpTable(DatasetName, rawTable.Name, DatasetMetadata, tables?.Value) { DisplayName = rawTable.DisplayName });
}

[Obsolete("Use GetTablesAsync without urlPrefix")]
public virtual async Task<IEnumerable<CdpTable>> GetTablesAsync(HttpClient httpClient, string uriPrefix, CancellationToken cancellationToken, ConnectorLogger logger = null)
{
if (DatasetMetadata == null)
Expand All @@ -50,6 +72,36 @@ public virtual async Task<IEnumerable<CdpTable>> GetTablesAsync(HttpClient httpC
return tables?.Value?.Select((RawTable rawTable) => new CdpTable(DatasetName, rawTable.Name, DatasetMetadata, tables?.Value) { DisplayName = rawTable.DisplayName });
}

/// <summary>
/// Retrieves a single CdpTable.
/// </summary>
/// <param name="httpClient">HttpClient.</param>
/// <param name="tableName">Table name to search.</param>
/// <param name="logicalOrDisplay">bool? value: true = logical only, false = display name only, null = logical or display name. All comparisons are case sensitive.</param>
/// <param name="cancellationToken">Cancellation token.</param>
/// <param name="logger">Logger.</param>
/// <returns>CdpTable class.</returns>
/// <exception cref="InvalidOperationException">When no or more than one tables are identified.</exception>
public virtual async Task<CdpTable> GetTableAsync(HttpClient httpClient, string tableName, bool? logicalOrDisplay, CancellationToken cancellationToken, ConnectorLogger logger = null)
{
cancellationToken.ThrowIfCancellationRequested();

IEnumerable<CdpTable> tables = await GetTablesAsync(httpClient, cancellationToken, logger).ConfigureAwait(false);
IEnumerable<CdpTable> filtered = tables.Where(ct => IsNameMatching(ct.TableName, ct.DisplayName, tableName, logicalOrDisplay));

if (!filtered.Any())
{
throw new InvalidOperationException("Cannot find any table with the specified name");
}

if (filtered.Count() > 1)
{
throw new InvalidOperationException($"Too many tables correspond to the specified name - Found {filtered.Count()} tables");
}

return filtered.First();
}

/// <summary>
/// Retrieves a single CdpTable.
/// </summary>
Expand All @@ -61,11 +113,12 @@ public virtual async Task<IEnumerable<CdpTable>> GetTablesAsync(HttpClient httpC
/// <param name="logger">Logger.</param>
/// <returns>CdpTable class.</returns>
/// <exception cref="InvalidOperationException">When no or more than one tables are identified.</exception>
[Obsolete("Use GetTableAsync without urlPrefix")]
public virtual async Task<CdpTable> GetTableAsync(HttpClient httpClient, string uriPrefix, string tableName, bool? logicalOrDisplay, CancellationToken cancellationToken, ConnectorLogger logger = null)
{
cancellationToken.ThrowIfCancellationRequested();
IEnumerable<CdpTable> tables = await GetTablesAsync(httpClient, uriPrefix, cancellationToken, logger).ConfigureAwait(false);

IEnumerable<CdpTable> tables = await GetTablesAsync(httpClient, uriPrefix, cancellationToken, logger).ConfigureAwait(false);
IEnumerable<CdpTable> filtered = tables.Where(ct => IsNameMatching(ct.TableName, ct.DisplayName, tableName, logicalOrDisplay));

if (!filtered.Any())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ protected internal static async Task<T> GetObject<T>(HttpClient httpClient, stri
protected internal static async Task<string> GetObject(HttpClient httpClient, string message, string uri, string content, CancellationToken cancellationToken, ConnectorLogger logger = null, [CallerMemberName] string callingMethod = "")
{
cancellationToken.ThrowIfCancellationRequested();
string log = $"{callingMethod}.{nameof(GetObject)} for {message}, Uri {uri}";
string log = $"{callingMethod}.{nameof(GetObject)} for {message}, Uri {(httpClient is PowerPlatformConnectorClient ppcc ? ppcc.RequestUrlPrefix : string.Empty)}{uri}";

try
{
Expand Down
Loading
Loading