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

Issue 2780 #2785

wants to merge 4 commits into from

Conversation

LucGenetier
Copy link
Contributor

No description provided.

@LucGenetier LucGenetier requested a review from a team as a code owner December 18, 2024 13:26
@LucGenetier LucGenetier linked an issue Dec 18, 2024 that may be closed by this pull request
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()

@LucGenetier
Copy link
Contributor Author

✅ No public API change.

@@ -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

@MikeStall
Copy link
Contributor

MikeStall commented Dec 18, 2024

    protected internal static async Task<T> GetObject<T>(HttpClient httpClient, string message, string uri, string content, CancellationToken cancellationToken, ConnectorLogger logger = null, [CallerMemberName] string callingMethod = "")

can we remove (coming from httpClient now).
Ditto for cases below.


Refers to: src/libraries/Microsoft.PowerFx.Connectors/Tabular/Services/CdpServiceBase.cs:21 in 7a35813. [](commit_id = 7a35813, deletion_comment = False)

@@ -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

@@ -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?

@LucGenetier
Copy link
Contributor Author

    protected internal static async Task<T> GetObject<T>(HttpClient httpClient, string message, string uri, string content, CancellationToken cancellationToken, ConnectorLogger logger = null, [CallerMemberName] string callingMethod = "")

This method is used in CdpDataSource to convert Http responses to POCOs
I don't understand why we'd remove it


In reply to: 2551745584


Refers to: src/libraries/Microsoft.PowerFx.Connectors/Tabular/Services/CdpServiceBase.cs:21 in 7a35813. [](commit_id = 7a35813, deletion_comment = False)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connectors shouldn't need both HttpClient and uriPrefix
2 participants