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.

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

fix: make preimage a nullable string #139

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion handle_payment_request.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ func (svc *Service) HandlePayInvoiceEvent(ctx context.Context, request *Nip47Req
},
}, ss)
}
payment.Preimage = preimage
payment.Preimage = &preimage
nostrEvent.State = NOSTR_EVENT_STATE_HANDLER_EXECUTED
svc.db.Save(&nostrEvent)
svc.db.Save(&payment)
Expand Down
7 changes: 7 additions & 0 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,13 @@ func main() {
log.Fatalf("Failed migrate DB %v", err)
}

// Update payments with preimage as an empty string to use NULL instead
// TODO: remove this migration in a future release and make preimage column unique
if err := db.Table("payments").Where("preimage = ?", "").Update("preimage", nil).Error; err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

don't do migrations like this.

either we have a proper migration setup or you do DB maintenance on the DB.

Copy link
Contributor

Choose a reason for hiding this comment

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

And if we don't have proper migrations. Then we must release this as a breaking change with details in the release notes, otherwise we break the deployment of others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I vote for a proper migration setup - I don't think we can release this as a breaking change. NWC on all umbrel apps will break and it's unreasonable to ask users to manually do something to fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gorm on its own does not seem to support doing a manual migration here (note: the string column from non-nullable to nullable IS migrated automatically, but not the query to update the preimage from '' to NULL needs to be done manually)

It looks like https://github.com/go-gormigrate/gormigrate could be the best option here, it runs inline (as opposed to atlas and golang-migrate)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think auto-migrate is mostly a dangerous option as you always have to be careful with the data that you have already in the DB.

and I think it's better to write explicit migrations then it is also possible to support different special SQL dialects (PG, SQLite)

log.Fatalf("Failed to update preimage from empty string to NULL: %v", err)
}


if cfg.NostrSecretKey == "" {
if cfg.LNBackendType == AlbyBackendType {
//not allowed
Expand Down
2 changes: 1 addition & 1 deletion models.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ type Payment struct {
NostrEvent NostrEvent
Amount uint
PaymentRequest string
Preimage string
Preimage *string
CreatedAt time.Time
UpdatedAt time.Time
}
Expand Down