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.

Sign up for GitHub

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

Jump to bottom

feat: Accept contentType params for DID URL Resolution [DEV-4722] #325

Closed
wants to merge 18 commits into from
Closed

feat: Accept contentType params for DID URL Resolution [DEV-4722] #325

wants to merge 18 commits into from

Conversation

DaevMithran
Copy link
Contributor

No description provided.

@ankurdotb
Copy link
Contributor

Task linked: DEV-4722 Apply Accept header logic for DID URL Dereferencing

@DaevMithran DaevMithran marked this pull request as draft January 31, 2025 09:21
@ankurdotb ankurdotb changed the title feat: Accept contetType params for DID URL Resolution feat: Accept contentType params for DID URL Resolution Jan 31, 2025
@ankurdotb ankurdotb changed the title feat: Accept contentType params for DID URL Resolution feat: Accept contentType params for DID URL Resolution [DEV-4722] Jan 31, 2025
@DaevMithran DaevMithran marked this pull request as ready for review February 6, 2025 10:14
@DaevMithran DaevMithran force-pushed the DEV-4722 branch 7 times, most recently from fa1dcad to 35be15c Compare February 6, 2025 13:26
@DaevMithran DaevMithran force-pushed the DEV-4722 branch 2 times, most recently from df4017b to 35169aa Compare February 6, 2025 13:36
ankurdotb
ankurdotb requested changes Feb 7, 2025
View reviewed changes
ankurdotb
ankurdotb requested changes Feb 12, 2025
View reviewed changes
Copy link
Contributor

@ankurdotb ankurdotb left a comment

Choose a reason for hiding this comment

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

Maybe the functionality of the overall task is meant to be delivered across different PR’s and I’m struggling to see the big picture, but AFAIK I don’t see anything in here that’s handling or testing what happens when the correct header/profile for URL dereferencing is supplied, but the resource is not JSON/text. (At the very least, it’s not in the tests.) Might be a function of no resources being created that are image or non-text/JSON perhaps?

"As-is" situations where only the resource is returned I'm less worried about.

services/helpers.go Outdated
@@ -39,3 +40,48 @@ func PrepareQueries(c echo.Context) (rawQuery string, flag *string) {
func GetDidParam(c echo.Context) (string, error) {
return url.QueryUnescape(c.Param("did"))
}

Copy link
Contributor

Choose a reason for hiding this comment

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

A separate note about function and variable names here: this is not getting the "header". It's specifically the Accept header. So it could be misleading that a lot of the function and variable names are called "header". I would suggest renaming functions and variables to call them "acceptHeader" or similar. (There's a similar comment I have made about variables called contentType below.)

services/resource_dereference_service.go Outdated
}

var context string
if contentType == types.DIDJSONLD || contentType == types.JSONLD {
Copy link
Contributor

Choose a reason for hiding this comment

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

Bit confused as to why this contentType is relevant for dereferencing? These are purely about DID Resolution?

As in...if it's a resource, will it ever be these two content types?

Side note: we seem to be using a lot of different terms: sometimes header (see previous comment on "header" vs "accept header"), sometimes contentType like here). It's quite variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should populdate context if it's jsonLD right. This condition is for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes you should, I was talking about inconsistent variable naming

tests/integration/rest/resource/data/positive_test.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm missing something, I don't see a test case here that would logically return "Accept: application/ld+json;profile="https://w3id.org/did-url-dereferencing" → Returns resource + metadata in a single response", since both of the test cases defined here would just return the resource which is JSON?

Copy link
Contributor

Choose a reason for hiding this comment

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

EDIT: Maybe just needs better test descriptions

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also like to see a test case where the Accept header is exactly the same string that a browser like Chrome sends, and cross-checking the results (since it doesn't have the DID URL dereferencing profile, you should not get resource + metadata back)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are added under data_with_metadata folder

Copy link
Contributor

Choose a reason for hiding this comment

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

Hence the "EDIT". The follow-up is about adding Chrome-like Accept header to test the "default" case of loading in a browser.

tests/integration/rest/resource/data_with_metadata/negative_test.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be test cases where there are completely invalid DID or resource IDs, but I don't see anything where the Accept headers are changed and test cases related to that?

Copy link
Contributor

Choose a reason for hiding this comment

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

EDIT: Maybe just needs better test descriptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when accept headers are different for /resource endpoint the tests are present with the data/ directory, it's not a failure

Only the tests for application/ld+json;profile="https://w3id.org/did-url-dereferencing" is within data_with_metadata

@DaevMithran DaevMithran force-pushed the DEV-4722 branch 2 times, most recently from 58fc643 to f2a826e Compare February 17, 2025 08:12
@DaevMithran
Copy link
Contributor Author

Duplicate of #340

@DaevMithran DaevMithran marked this as a duplicate of #340 Feb 20, 2025
@ankurdotb ankurdotb deleted the DEV-4722 branch February 25, 2025 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ankurdotb ankurdotb ankurdotb requested changes

@Eengineer1 Eengineer1 Awaiting requested review from Eengineer1

@sownak sownak Awaiting requested review from sownak

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DaevMithran @ankurdotb @sownak @Eengineer1