Skip to content

Conversation

@nicolaslazo
Copy link
Contributor

Description:

This PR implements a new filesource connector for Microsoft SharePoint. It uses the Graph SDK to build the API requests needed and Go's OAuth module for authentication. Though the SDK does implement some authentication methods, none allow for the use of refresh tokens.

Two different configuration methods are supported. Users may omit library names if they intend to use the site's default library, and the values could be parsed from the URL displayed when they navigate to the target directory through the web UI.

Workflow steps:

(How does one use this feature, and how has it changed)

Documentation links affected:

(list any documentation links that you created, or existing ones that you've identified as needing updates, along with a brief description)

Notes for reviewers:

This connector has not been tested locally due to lacking access to a SharePoint subscription. However, most of the logic has been tested through the OneDrive sibling connector.

@nicolaslazo nicolaslazo requested a review from Copilot November 27, 2025 22:21
@nicolaslazo nicolaslazo self-assigned this Nov 27, 2025
Copilot finished reviewing on behalf of nicolaslazo November 27, 2025 22:23
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a new filesource connector for Microsoft SharePoint that enables reading files from SharePoint document libraries. The connector leverages the Microsoft Graph SDK for API interactions and Go's OAuth2 module for authentication with refresh token support.

  • Implements SharePoint site and document library access via Graph API
  • Supports flexible configuration through either site URL or site ID
  • Includes comprehensive URL parsing and validation for both /sites/ and /teams/ URLs

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
source-sharepoint/main.go Core connector implementation including SharePoint Graph API integration, configuration validation, URL parsing, and file listing/reading logic
source-sharepoint/oauth2.go OAuth2 authentication implementation using refresh tokens with Microsoft identity platform
source-sharepoint/main_test.go Unit tests for spec generation and SharePoint URL parsing with comprehensive test cases
source-sharepoint/Dockerfile Multi-stage Docker build configuration for the connector
source-sharepoint/VERSION Version file marking initial v1 release
source-sharepoint/CHANGELOG.md Changelog documenting the initial release
source-sharepoint/.snapshots/TestSpec Snapshot test file for configuration schema validation
source-sharepoint/tests/parser_spec.json Parser configuration schema for file format handling

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@nicolaslazo nicolaslazo marked this pull request as draft November 27, 2025 22:28
@nicolaslazo nicolaslazo force-pushed the nlazo/source-sharepoint branch from af177a5 to 6729e27 Compare November 28, 2025 14:12
@nicolaslazo nicolaslazo requested a review from Copilot November 28, 2025 14:22
Copilot finished reviewing on behalf of nicolaslazo November 28, 2025 14:25
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@@ -0,0 +1,5 @@
# source-sharepoint

## v1, 2025-11-26
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The changelog date shows 2025-11-26, but this appears to be a typo. Based on the current date context (November 2025), this should likely be 2024-11-26.

Suggested change
## v1, 2025-11-26
## v1, 2024-11-26

Copilot uses AI. Check for mistakes.
Comment on lines +450 to +456
if name := firstUnprocessedItem.GetName(); name != nil {
if folder.path == "" {
fullItemPath = "/" + *name
} else {
fullItemPath = folder.path + "/" + *name
}
}
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Potential bug: If GetName() returns nil, fullItemPath will remain an empty string. This empty path would then be used in subsequent operations like API calls (root::) or path comparisons, which could cause unexpected behavior or errors. Consider adding a check to skip or handle items with nil names:

name := firstUnprocessedItem.GetName()
if name == nil {
    // Skip items without names or log a warning
    continue
}
fullItemPath := folder.path + "/" + *name
if folder.path == "" {
    fullItemPath = "/" + *name
}
Suggested change
if name := firstUnprocessedItem.GetName(); name != nil {
if folder.path == "" {
fullItemPath = "/" + *name
} else {
fullItemPath = folder.path + "/" + *name
}
}
name := firstUnprocessedItem.GetName()
if name == nil {
// Skip items without names
continue
}
if folder.path == "" {
fullItemPath = "/" + *name
} else {
fullItemPath = folder.path + "/" + *name
}

Copilot uses AI. Check for mistakes.
fmt.Errorf("error resolving site ID for %s: %w", siteIdentifier, err)
}

return *site.GetId(), nil
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Potential bug: Dereferencing site.GetId() without checking for nil could cause a panic if the API returns a site without an ID. Consider adding a nil check:

siteID := site.GetId()
if siteID == nil {
    return "", fmt.Errorf("site %s returned no ID", siteIdentifier)
}
return *siteID, nil
Suggested change
return *site.GetId(), nil
siteID := site.GetId()
if siteID == nil {
return "", fmt.Errorf("site %s returned no ID", siteIdentifier)
}
return *siteID, nil

Copilot uses AI. Check for mistakes.
return "", fmt.Errorf("error fetching site's default drive: %w", err)
}

return *drive.GetId(), nil
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Potential bug: Dereferencing drive.GetId() without checking for nil could cause a panic if the API returns a drive without an ID. Consider adding a nil check:

driveID := drive.GetId()
if driveID == nil {
    return "", fmt.Errorf("default drive returned no ID")
}
return *driveID, nil
Suggested change
return *drive.GetId(), nil
driveID := drive.GetId()
if driveID == nil {
return "", fmt.Errorf("default drive returned no ID")
}
return *driveID, nil

Copilot uses AI. Check for mistakes.

for _, drive := range drives.GetValue() {
if drive.GetName() != nil && *drive.GetName() == libraryName {
return *drive.GetId(), nil
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Potential bug: Dereferencing drive.GetId() without checking for nil could cause a panic. While the code checks that drive.GetName() is not nil, it doesn't verify drive.GetId() before dereferencing. Consider adding a nil check:

if drive.GetName() != nil && *drive.GetName() == libraryName {
    driveID := drive.GetId()
    if driveID == nil {
        return "", fmt.Errorf("drive '%s' returned no ID", libraryName)
    }
    return *driveID, nil
}
Suggested change
return *drive.GetId(), nil
driveID := drive.GetId()
if driveID == nil {
return "", fmt.Errorf("drive '%s' returned no ID", libraryName)
}
return *driveID, nil

Copilot uses AI. Check for mistakes.
@nicolaslazo nicolaslazo marked this pull request as ready for review November 28, 2025 14:29
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.

2 participants