-
Notifications
You must be signed in to change notification settings - Fork 10
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
support web did resolution over http for local host #325
support web did resolution over http for local host #325
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #325 +/- ##
=======================================
Coverage 73.60% 73.60%
=======================================
Files 45 45
Lines 2163 2163
Branches 402 403 +1
=======================================
Hits 1592 1592
Misses 376 376
Partials 195 195
|
@@ -192,15 +192,15 @@ public sealed class DidWebApi( | |||
private fun decodeId(parsedDid: Did): String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aparkersquare thoughts on using a parsed url to build targetUrl
? something like
public fun decodeId(parsedDid: Did): String {
val domainNameWithPath = parsedDid.id.replace(":", "/")
val decodedDomain = URLDecoder.decode(domainNameWithPath, UTF_8)
val url = URL("https://$decodedDomain")
val decodedId = StringBuilder()
if (url.host == "localhost" || url.host == "127.0.0.1") {
decodedId.append("http://")
} else {
decodedId.append("https://")
}
decodedId.append(decodedDomain)
if (url.path.isEmpty()) {
decodedId.append(WELL_KNOWN_URL_PATH)
}
decodedId.append(DID_DOC_FILE_NAME)
return decodedDomain.toString()
}
potential benefits of doing it this way are:
- url is parsed (therefore "validated" prior to doing anything else
- we can check
url.host
explicitly
there's probably kotlin idioms that can be used in the above suggestion to make the code more kotlin-esque which you'd know better about than me!
when i gave chad (aka chatGPT) the above suggestion and asked him for something more idiomatic, he suggested:
public fun decodeId(parsedDid: Did): String {
val domainNameWithPath = parsedDid.id.replace(":", "/")
val decodedDomain = URLDecoder.decode(domainNameWithPath, UTF_8.name())
val url = URL("https://$decodedDomain")
return buildString {
append(if (url.host == "localhost" || url.host == "127.0.0.1") "http://" else "https://")
append(decodedDomain)
if (url.path.isEmpty()) {
append(WELL_KNOWN_URL_PATH)
}
append(DID_DOC_FILE_NAME)
}
}
let me know what you think! i personally prefer the former but maybe that's because i don't write much kotlin. Either way, 👍🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooh, I didn't know about url.host
!
Agree, that looks like it could be a nicer approach. Thanks.
I'll have a closer look soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works.
Updated the PR to use this approach, and deleted the LocalHostDomainMatcher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naisuu!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @aparkersquare ! left a comment and approved regardless so you're not stuck waiting for approval
b256de5
to
7ae58e7
Compare
thanks so much @aparkersquare 🙏🏾 |
thank you so much! |
Overview
Uses
http
for web did resolution for local hosts. This allows for local development.See #324