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

GetFolderByServerRelativeUrlAsync not working anymore with path from System.IO.Path [regression?] #1412

Closed
1 task done
FlorianLabranche opened this issue Mar 5, 2024 · 10 comments
Assignees
Labels
question Further information is requested

Comments

@FlorianLabranche
Copy link

Category

  • Bug

Describe the bug

Retrieving folder from path handled using System.IO.Path was working fine in PnP.Core 1.10
When upgrading SDK to 1.12, the same code returns a 404 'File Not Found.' exception.

Steps to reproduce

Below code snippet returns an exception in SDK 1.11 and 1.12 but works fine in 1.10

var directoryPath = Path.GetDirectoryName(file.ServerRelativeUrl);
var folder = await PnPContext.Web.GetFolderByServerRelativeUrlAsync(directoryPath);

This snippet is only to demonstrate the issue. In this case, to retrieve the folder, I would use file.ListItemAllFields.GetParentFolderAsync().

Expected behavior

I don't know if this should be handled by the library because the issue is related to the path being encoded by System.IO.Path.
Backslashes are escaped.
image

But as this was working in a previous release, I raise it as a bug.

Environment details (development & target environment)

  • SDK version: 1.12
  • OS: Windows 10
  • SDK used in: ASP.Net Web app
  • Framework: .Net 6
  • Tooling: Visual Studio 2022

Additional context

Code is working once deployed in a Linux Web App.
But path are handled differently in linux.

@jansenbe
Copy link
Contributor

jansenbe commented Mar 5, 2024

@FlorianLabranche : assume there's no difference in casing between testing with 1.11 and 1.12?

@jansenbe jansenbe added the question Further information is requested label Mar 5, 2024
@FlorianLabranche
Copy link
Author

No difference.
1.10 :    "\sites\DMS_prj_90480\Correspondence" (working)
1.11/1.12 :  "\sites\DMS_prj_90480\Correspondence" (not working)

@jansenbe
Copy link
Contributor

jansenbe commented Mar 6, 2024

@FlorianLabranche : can you do a Fiddler trace and compare both GET requests, spotting the difference there will help identify the root cause. Also, does it fail on Linux, on Windows or both?

@jansenbe
Copy link
Contributor

jansenbe commented Mar 8, 2024

@FlorianLabranche : looking at the code this is the most recent change here, would not expect this to result in what you see...

image

@jansenbe
Copy link
Contributor

jansenbe commented Mar 8, 2024

@FlorianLabranche : does it also happen with sites that do not have an _ in their name?

@jansenbe
Copy link
Contributor

@FlorianLabranche : any updates here?

@jansenbe jansenbe self-assigned this Mar 13, 2024
@FlorianLabranche
Copy link
Author

@jansenbe here are the results of my tests :

  • 1.10

/sites/DMS_prj_90480/_api/Web/getFolderByServerRelativePath(decodedUrl='%5Csites%5CDMS_prj_90480%5CCorrespondence')

  • 1.11 or 1.12

/sites/DMS_prj_90480/_api/Web/getFolderByServerRelativePath(decodedUrl=@u)?@u='%5Csites%5CDMS_prj_90480%5CCorrespondence'

The encoded path is the same, so '\' is not an issue.
In the second case, if I replace '\\' by '/' before I make the call, it works with the following URL:
/sites/DMS_prj_90480/_api/Web/getFolderByServerRelativePath(decodedUrl=@u)?@u='%2Fsites%2FDMS_prj_90480%2FCorrespondence'

Targeting sites with or without _ in their url has no impact.

Issue seems to come from the change you shared.

@jansenbe
Copy link
Contributor

@FlorianLabranche : interesting, tested this and so it seems that using the "parameter" model requires the server relative path to use / while the previous one worked with either / and . I'll update the code to handle that automatically

@FlorianLabranche
Copy link
Author

It's weird they behave differently, indeed.
Many thanks

jansenbe added a commit that referenced this issue Mar 13, 2024
@jansenbe
Copy link
Contributor

@FlorianLabranche : pushed an update, thanks for raising this one :-)

Closing issue now, please re-open if things don't seem to work

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

No branches or pull requests

2 participants