-
Notifications
You must be signed in to change notification settings - Fork 140
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
Lexicon resolution #792
Lexicon resolution #792
Conversation
d60f92f
to
12f4572
Compare
b50b014
to
e0ffe1c
Compare
This is ready for review/merge |
Will add this to 'goat' instead
e0ffe1c
to
6ac44d9
Compare
ErrNSIDNotFound = fmt.Errorf("NSID not associated with a DID") | ||
) | ||
|
||
// Resolves an NSID to a DID, as used for Lexicon resolution (using "_lexicon" DNS TXT record) |
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.
no option for https://*/.well-known/atproto-lexicon
?
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.
nope. we discussed/debated and decided against. I think there is some discussion in: bluesky-social/atproto#3074
atproto/lexicon/catalog.go
Outdated
return nil | ||
} | ||
slog.Debug("loading Lexicon schema file", "path", p) | ||
f, err := os.Open(p) |
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.
os.Open()
won't work for the LoadEmbedFS() case
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.
ok, i'll add a test to cover that use-case
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.
think I got this fixed. thanks for the catch!
baseDir := identity.BaseDirectory{} | ||
did, err := baseDir.ResolveNSID(ctx, nsid) | ||
if err != nil { | ||
return nil, err | ||
} |
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.
Is there a reason we use the BaseDirectory here and not the identity directory? I guess to avoid adding ResolveNSID as a required method on the directory interface for now?
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.
Never mind I see we suggest you use ResolvingCatalog
anyhow
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.
yeah, basically putting that method on BaseDirectory
to start. would probably make sense to add to caching and other implementations, and thus the interface, but doing baby steps.
Directory: identity.DefaultDirectory(), | ||
} | ||
} | ||
|
||
func (rc *ResolvingCatalog) Resolve(ref string) (*Schema, error) { |
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.
Can this string be anything? Should it be a URL or something and should we validate that?
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.
it is a "Lexicon reference", which is an NSID optionally followed by name-fragment. I'm planning to add a syntax parser for that identifier, but still hashing out the specification of the syntax (what we have written on atproto.com is probably too restrictive).
Adds a
ResolveNSID
method toidentity.BaseDirectory
. I think keeping DNS TXT resolution stuff together in the same package makes sense, even if this is a "lexicon" thing, not really an "identity" thing. Could consider renaming that method toResolveLexiconNSID
or similar, to clarify it is about NSIDs in the context of Lexicons, not "NSIDs in general" (though they are currently only defined in terms of Lexicons).Adds a new
ResolvingCatalog
implementation of thelexicon.Catalog
interface, which does resolution from the live network and adds to a local cache. Alsolexicon.ResolveLexiconData
low-level helper which goes from NSID to Lexicon record data.