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

Fix: content-negotiation when fetching dataset from url #1153

Merged
merged 17 commits into from
Feb 15, 2022

Conversation

Falx
Copy link
Contributor

@Falx Falx commented Feb 10, 2022

πŸ“ Related issues

#1039

✍️ Description

The reasoning behind this fix was to only use rdf-dereference when the input type is string. That is where the error seems to occur, other flows seem fine. (I might be wrong though)

βœ… PR check list

Before this pull request can be merged, a core maintainer will check whether

  • this PR is labeled with the correct semver label
    • semver.patch: Backwards compatible bug fixes.
    • semver.minor: Backwards compatible feature additions.
    • semver.major: Breaking changes. This includes changing interfaces or configuration behaviour.
  • the correct branch is targeted. Patch updates can target main, other changes should target the latest versions/* branch.
  • the RELEASE_NOTES.md document in case of relevant feature or config changes.

src/util/FetchUtil.ts Outdated Show resolved Hide resolved
Copy link
Member

@joachimvh joachimvh left a comment

Choose a reason for hiding this comment

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

The integration tests succeed so that means that the code actually works. The unit tests are going to fail though because of the changes. The big one is that it is now trying to do actual HTTP requests instead of mock requests. You'll have to mock the rdf-dereference library now instead and probably do a few other changes there as well https://github.com/solid/community-server/blob/5e0f2e1e9582de6f853f60428ca086bccdff178c/test/unit/util/FetchUtil.test.ts#L8

src/util/FetchUtil.ts Outdated Show resolved Hide resolved
src/util/FetchUtil.ts Outdated Show resolved Hide resolved
@Falx Falx marked this pull request as ready for review February 14, 2022 08:19
Copy link
Member

@joachimvh joachimvh left a comment

Choose a reason for hiding this comment

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

The error message made me realize where the problem is. I pulled the branch and verified the suggested changes work. There is still a test error but it's not related to the mock/types any more. My suggestions can probably also be cleaned up a bit if needed.

test/unit/util/FetchUtil.test.ts Outdated Show resolved Hide resolved
test/unit/util/FetchUtil.test.ts Show resolved Hide resolved
test/unit/util/FetchUtil.test.ts Outdated Show resolved Hide resolved
@Falx
Copy link
Contributor Author

Falx commented Feb 14, 2022

I think it looks good now.

@Falx Falx changed the title Solution works but tests don't Fix: content-negotiation when fetching dataset from url Feb 14, 2022
@Falx Falx changed the base branch from main to versions/3.0.0 February 14, 2022 11:09
@Falx
Copy link
Contributor Author

Falx commented Feb 14, 2022

Force pushed the new changes

@Falx Falx added the semver.major Requires a major version bump label Feb 14, 2022
Copy link
Member

@joachimvh joachimvh left a comment

Choose a reason for hiding this comment

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

Some minor issues remaining.

package-lock.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/util/FetchUtil.ts Outdated Show resolved Hide resolved
src/util/FetchUtil.ts Outdated Show resolved Hide resolved
src/util/FetchUtil.ts Outdated Show resolved Hide resolved
test/unit/util/FetchUtil.test.ts Outdated Show resolved Hide resolved
@Falx Falx requested a review from joachimvh February 15, 2022 10:19
Copy link
Member

@joachimvh joachimvh left a comment

Choose a reason for hiding this comment

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

Some very minor style nitpicks, will merge after those.

src/authorization/access/AgentGroupAccessChecker.ts Outdated Show resolved Hide resolved
src/util/FetchUtil.ts Outdated Show resolved Hide resolved
src/util/FetchUtil.ts Outdated Show resolved Hide resolved
@Falx
Copy link
Contributor Author

Falx commented Feb 15, 2022

That should do it then

@Falx Falx requested a review from joachimvh February 15, 2022 12:30
Copy link
Member

@joachimvh joachimvh left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@joachimvh joachimvh merged commit ce754c1 into versions/3.0.0 Feb 15, 2022
@joachimvh joachimvh deleted the feat/fetch-dataset-headers branch February 15, 2022 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver.major Requires a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants