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

Transaction management #2

Merged
merged 52 commits into from
Nov 2, 2023
Merged

Transaction management #2

merged 52 commits into from
Nov 2, 2023

Conversation

Chystik
Copy link
Owner

@Chystik Chystik commented Oct 30, 2023

No description provided.

Makefile Outdated
@@ -0,0 +1,56 @@
#SHELL = /bin/bash
.PHONY: autotest dep test race lint gen cover statictest dev-up dev-down accrual dev-autotest
Copy link
Collaborator

Choose a reason for hiding this comment

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

PHONY лучше прописывать отдельно перед каждым таргетом.

Будет легче разрешать конфликты при мерджах + при большом кол-ве таргетов становится неудобно поддерживать общий PHONY

Copy link
Owner Author

Choose a reason for hiding this comment

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

прописал перед каждым

config/app.go Outdated

type (
App struct {
Address `env:"RUN_ADDRESS"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Встраивание не для наследования полей, а для наследования поведения. Лучше делать обычные поля, т.к. встраивание может начать путать и приводить к ошибкам при вызове методов.

Copy link
Owner Author

Choose a reason for hiding this comment

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

поправил

}
}

func errorJSON(w http.ResponseWriter, err error, status int, logger logger.AppLogger) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Статус лучше определять по типу ошибки с использованием конструкции errors.Is

Copy link
Owner Author

Choose a reason for hiding this comment

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

теперь все коды сопоставляются с ошибками

Data interface{} `json:"data,omitempty"`
}

func writeJSON(w http.ResponseWriter, status int, contentType string, data any, logger logger.AppLogger, headers ...http.Header) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Не стоит передавать сюда content-type, всегда будет json

Copy link
Owner Author

Choose a reason for hiding this comment

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

убрал

var ctx = context.Background()
var err error

orderNumberRaw, err = io.ReadAll(r.Body)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Нужно закрывать тело обязательно

Copy link
Owner Author

Choose a reason for hiding this comment

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

поправил

}

func (s *syncer) Run() error {
s.tick <- struct{}{} // init tick
Copy link
Collaborator

Choose a reason for hiding this comment

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

Лучше использовать time.Ticker

Copy link
Owner Author

Choose a reason for hiding this comment

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

переписал с использованием time.Ticker

}

func (s *syncer) Shutdown(ctx context.Context) error {
once.Do(func() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

once должна быть переменной syncer, а не модуля, т.к. исходя из дизайна ликеров может быть несколько

Copy link
Owner Author

Choose a reason for hiding this comment

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

поправил

}

func (s *syncer) sync() error {
orders, err := s.orderRepo.GetUnprocessed(context.TODO())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Контекст TODO не должен присутствовать в итоговом коде

Copy link
Owner Author

Choose a reason for hiding this comment

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

Упустил. Исправил.

type User struct {
Login string `json:"login" db:"login"`
Password string `json:"password" db:"password"`
Balance float64 `db:"balance"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Не стоит работать с денежными единицами во float64. Из-за неточности арифметических операций могут потеряться копейки и быть юридические проблемы

Copy link
Owner Author

Choose a reason for hiding this comment

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

Теперь храню в

type Money struct {
	Amount Amount
}

type Amount uint64


// if the new status is valid for accrual - change user balance
if newStatus == "PROCESSED" {
user, err := s.userRepo.Get(ctx, models.User{Login: orders[i].User})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Без установки блокировки возможна потеря списаний.

Например, когда пользователь запрашивает списание баллов и мы одновременно обрабатываем его заказ.

Необходимо выполнять списание баллов в транзакции.

Copy link
Owner Author

Choose a reason for hiding this comment

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

запихнул в транзакцию с использованием менеджера транзакций

@Chystik Chystik requested a review from popoffvg November 2, 2023 11:37
@Chystik Chystik merged commit 6a32a60 into main Nov 2, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants