Include full URLs in search results #31
Labels
No labels
area: app c
area: app d
area: devops
area: discovery
area: docs
area: proposal
area: X-device Sync
Chainquery
consider soon
dependencies
Epic
Fix till next release
good first issue
hacktoberfest
help wanted
icebox
Invalid
level: 1
level: 2
level: 3
level: 4
needs: exploration
needs: grooming
needs: priority
needs: repro
needs: tech design
on hold
Parked
priority: blocker
priority: high
priority: low
priority: medium
Tom's Wishlist
type: bug
type: discussion
type: improvement
type: new feature
type: refactor
type: task
type: testing
unplanned
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: LBRYCommunity/lighthouse.js#31
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
URLs are typically what will ultimately be resolved or used by a service or application utilizing the search API. So it'd probably make sense to just return these.
@kauffj I wanted to just get this one done, however, there could be dependencies, and there may be alternative uses of getting claim value details based on a search result. Either way it returns all that it does because the data is stored in elastic search as an elastic document that can be summed up as a json object of information. The results are for these documents. If we wanted to return the URLs it would just be some minor post processing.
Since there are external dependencies on this API it is probably best to create a new API:
current http://localhost:50005/search?s=
new http://localhost:50005/url/search?s=
This way we can have the service live before it is used live by the app and the current one can still be used unhindered until the adjustment in the app is ready. Also it reduces golive dependencies. The drawback is now we have 2 apis.
I was going through tickets assigned to me and realized I never saw above until now. Good catch of a real issue, but I'd propose the proper fix for this is likely to add a version parameter the endpoint to allow graceful migration, not diverge endpoints.
Yeah that makes sense! Will do
Looking here it appears we use the name and claim id to build a URI and that is what gets used. So we do not actually need another api, we can just return these things. However, for debugging the api, this additional information is valuable.
New results without sending this information is a lot smaller.
@seanyesmunt we have the PR ready, but I just wanted to double check with you that we are not using the search APIs anywhere else.
They are used in the android app. It would definitely be a smoother transition to include versioning in the api
I'm not 100% on what I meant by this, but I think I just meant to actually include the URL so it doesn't have to be reconstructed, not to actually strip out the metadata (though that may be fine).
Ideally, it'd return the canonical url.
@seanyesmunt Yeah I agree. However, in the link I posted for lbry-redux (both mobile and desktop) we are only using those two values. So I don't think this versioning work needs be done right now.
@kauffj I would think parsing these two pieces of information is more costly than concatenating where the api is used. I feel like the search api should just give the raw information (name and claimId) needed with as little work as possible. It makes the API more flexible for future usage. Also, showing the full claim information also makes it as flexible as possible. I would ultimately, be in favor of leaving the API unchanged in case we want to use this information in the future.
It's still not performant to display/determine the shortest possible canonical URL, but I'm fine waiting on the daemon to fix that.
That said, even without proper canonical URLs, I do think it makes sense to return the full URL. It'd be like if a search engine returned the host and TLD separately (technically host includes TLD but you get what I mean).
@tiger5226 Whoops. You are right. I thought we were handling that data differently.
All clear. I will leave the current fields alone and add one for the canonical url which should be:
<claim_name>#<claim_id>is this correct @kauffj ?I reviewed the issue linked, but I don't see what exactly we want the canonical url to be. Sorry if I missed something.
@tiger5226 no, the canonical URL is more complex than that. Given full access to chainquery however, it should be possible to compute it in a single query/pass.
I updated https://github.com/lbryio/lbry/issues/958 to (hopefully) be significantly more clear
@kauffj This seems very complex. In a given query each row would have to compare against all other rows with a similar set of fields. Do you think this is something I should adjust Chainquery to calculate when a new claim comes in? Also what is the impact if a claim is abandoned? I presume that doesn't matter. If it existed in the blockchain at some point it impacts the canonical url. Is this correct? Adjusting Chainquery seems like a good idea. I could see someone using the API to gain quick access to the canonical URL or getting the claimid from a canonical url. If chainquery has already done these calculations it will be much faster. Also the adjustment to the lighthouse query becomes super simple as well. We just add a field to grab and push to elastic search.
@tiger5226 canonical URLs cannot (and won't) change even if a claim is abandoned.
We can probably wait to handle this in lighthouse until it's been added at lower levels.
ok I will put this in the icebox.
@tiger5226 Is this ready for review or going to the icebox?
icebox, corrected the pipeline to triaged.
closing until it's been added at a lower level