Outlook Mail: map Outlook IDs to auto-incrementing numerical IDs #46
Conversation
Signed-off-by: Grant Linville <[email protected]>
| go 1.23.0 | ||
|
|
||
| require ( | ||
| github.com/glebarez/sqlite v1.11.0 |
There was a problem hiding this comment.
This is the pure Go (no CGo) implementation of the SQLite driver for GORM.
Signed-off-by: Grant Linville <[email protected]>
g-linville
requested review from
thedadams,
njhale,
StrongMonkey,
iwilltry42 and
tylerslaton
| @@ -2,8 +2,11 @@ module github.com/gptscript-ai/tools/apis/outlook/mail/code | |||
|
|
|||
| go 1.23.0 | |||
|
|
|||
| replace github.com/gptscript-ai/tools/apis/outlook/common => github.com/g-linville/tools/apis/outlook/common v0.0.0-20240903203431-d7bfd26dc70d | |||
There was a problem hiding this comment.
This is pointing at my fork for now. Unfortunately I can't really change this until after I merge this PR. Should I open up a second PR to introduce the common package by itself first?
There was a problem hiding this comment.
I think it's fine to merge and immediately create another PR.
There was a problem hiding this comment.
any reason this common package has to be separate go module?
There was a problem hiding this comment.
I had separate Go modules for the manage and read tools, and want this code to be usable by both of them without one of them being the distinct "owner" of it
| } else { | ||
| printers.PrintMessages(messages, false) | ||
| return printers.PrintMessages(messages, false) |
There was a problem hiding this comment.
nit: don't need the else block anymore.
| @@ -74,3 +82,47 @@ func PrintMessage(msg models.Messageable, detailed bool) error { | |||
| func recipientableToString(r models.Recipientable) string { | |||
| return fmt.Sprintf("%s (%s)", util.Deref(r.GetEmailAddress().GetName()), util.Deref(r.GetEmailAddress().GetAddress())) | |||
| } | |||
|
|
|||
| func fixMailFolder(folder *models.MailFolderable) error { | |||
There was a problem hiding this comment.
I would appreciate a more descriptive name for this, or a comment explaining exactly what is being fixed.
There was a problem hiding this comment.
Yeah I should have named these better lol
| return nil | ||
| } | ||
|
|
||
| func fixMessage(msg *models.Messageable) error { |
| return fmt.Errorf("failed to set folder ID: %w", err) | ||
| } | ||
|
|
||
| (*folder).SetId(util.Ptr(newFolderID)) |
There was a problem hiding this comment.
Hmm I think folder.SetId should just work?
There was a problem hiding this comment.
No clue why, but it wasn't working, which is why I had to do it this way.
Signed-off-by: Grant Linville <[email protected]>
| // replaceMailFolderIDs replaces the ID values of the mail folder itself and its parent | ||
| // with the corresponding numerical ID that we generate in the database. | ||
| // This is necessary to do prior to printing it for the LLM. | ||
| func replaceMailFolderIDs(folder *models.MailFolderable) error { |
There was a problem hiding this comment.
Daishan's comment below caused me to look into this: models.MailFolderable is an interface, so you're passing a pointer to an interface (which is why you have to dereference to use it).
Since you are "addressing" the variable yourself when you pass to these "replace IDs" functions, I would suggest not doing that and passing the variable itself (which is likely already a pointer because it's methods are mutating).
| // with the corresponding numerical ID that we generate in the database. | ||
| // This is necessary to do prior to printing it for the LLM. | ||
| func replaceMailFolderIDs(folder *models.MailFolderable) error { | ||
| if folder == nil { |
There was a problem hiding this comment.
Because of the way Go's type system works, after making the changes suggested above, this nil check doesn't really make sense. It won't protect you from a nil-pointer dereference.
You may already know that, but just in case: https://go.dev/play/p/FGVP-LqqzpT
Signed-off-by: Grant Linville <[email protected]>
Signed-off-by: Grant Linville <[email protected]>
gpt-4o was getting confused by the long base64 IDs that Outlook uses. This change swaps all of those for auto-incrementing numerical IDs, which are much easier for the LLM to handle. We store the mapping in a SQLite DB stored in the workspace directory, and we create new entries on the fly as needed. The mapping is indexed on both fields so we can do reverse lookups performantly as well, to avoid unnecessarily creating the same entry in the mapping multiple times.
For now, this is implemented only in Mail, but I made the mapping stuff its own package so that we can easily include it in Calendar. Based on my testing, this seems to work very well, but I want a couple other people to try it before I bother to do it in the Calendar code.