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

feat: keep result as nil when record not found & dest is nil #6958

Closed
wants to merge 4 commits into from

Conversation

lcgash
Copy link

@lcgash lcgash commented Apr 11, 2024

  • [✔️ ] Do only one thing
  • Non breaking API changes
  • [✔️ ] Tested

What did this pull request do?

User Case Description

当前版本的实现,在下面的代码里面,app 经过了 orm 之后从原本的值 nil 变成了一个非 nil 的结果(当没有查询到数据时)

func GetByName(name string) (*model.App, error) {
	var app *model.App
	err := db.Where(`name = ?`, name).First(&app).Error
	return app, err
}

当下面的函数进行调用时,我们需要判断是否有数据,仅仅判定不为 nil 是不可行的, 通常又不太想直接写 是否为gorm.ErrRecordNotFound,因为这个 model 数据很有可能传递给了另外一个函数,没有上下文的情况下只能进行 nil 的判定

app, err := store.App.GetByName(name)
if err != nil && !errors.Is(err, gorm.ErrRecordNotFound) {
       return
}
if app != nil && app.ID != 0 {
     // do something
}

@demoManito
Copy link
Member

demoManito commented Apr 12, 2024

Hello, why not put the judgment of errors.Is(err, gorm.ErrRecordNotFound) in GetByName?

func GetByName(name string) (*model.App, error) {
	var app *model.App
	err := db.Where(`name = ?`, name).First(&app).Error
	if err != nil {
		if errors.Is(err, gorm.ErrRecordNotFound) {
			return nil, nil
		}
		return nil, err
	}
	return app, nil
}

@lcgash
Copy link
Author

lcgash commented Apr 12, 2024

Hello, why not put the judgment of errors.Is(err, gorm.ErrRecordNotFound) in GetByName?

func GetByName(name string) (*model.App, error) {
	var app *model.App
	err := db.Where(`name = ?`, name).First(&app).Error
	if err != nil {
		if errors.Is(err, gorm.ErrRecordNotFound) {
			return nil, nil
		}
		return nil, err
	}
	return app, nil
}

这样判定这种是个解决办法,但是这个本身是nil的元素进入了这个查询以后更改成了不为nil,这个行为本身是不是符合预期呢,感觉nil进去了以后在面对找不到的时候仍是nil更符合预期?

@lcgash
Copy link
Author

lcgash commented Apr 15, 2024

Hello, why not put the judgment of errors.Is(err, gorm.ErrRecordNotFound) in GetByName?

func GetByName(name string) (*model.App, error) {
	var app *model.App
	err := db.Where(`name = ?`, name).First(&app).Error
	if err != nil {
		if errors.Is(err, gorm.ErrRecordNotFound) {
			return nil, nil
		}
		return nil, err
	}
	return app, nil
}

重构了下,感觉这样应该是合理的?保持用户传递进入的 nil 或者其他值 在无法找到数据的情况下,应该是原样的?

@lcgash lcgash changed the title feat: support reset result as nil when record not found feat: keep result as nil when record not found & dest is nil Apr 15, 2024
@lcgash
Copy link
Author

lcgash commented Apr 15, 2024

Hello, why not put the judgment of errors.Is(err, gorm.ErrRecordNotFound) in GetByName?

func GetByName(name string) (*model.App, error) {
	var app *model.App
	err := db.Where(`name = ?`, name).First(&app).Error
	if err != nil {
		if errors.Is(err, gorm.ErrRecordNotFound) {
			return nil, nil
		}
		return nil, err
	}
	return app, nil
}

重构了下,感觉这样应该是合理的?保持用户传递进入的 nil 或者其他值 在无法找到数据的情况下,应该是原样的?

@jinzhu jinzhu大佬,辛苦帮忙看看,这个改动是否有必要或者合理

@jinzhu
Copy link
Member

jinzhu commented Apr 15, 2024

感觉这个修改没有啥必要,因为字段的初始化也有类似的问题~

@lcgash
Copy link
Author

lcgash commented Apr 15, 2024

感觉这个修改没有啥必要,因为字段的初始化也有类似的问题~

字段初始化目前是在哪里有什么问题么,暂时还没发现,目前就发现 nil 进去了出来就不是 nil 了

@jinzhu
Copy link
Member

jinzhu commented Apr 15, 2024

感觉这个修改没有啥必要,因为字段的初始化也有类似的问题~

字段初始化目前是在哪里有什么问题么,暂时还没发现,目前就发现 nil 进去了出来就不是 nil 了

记得有一些 preload 应该是这样子的,或者嵌套字段

@lcgash
Copy link
Author

lcgash commented Apr 15, 2024

感觉这个修改没有啥必要,因为字段的初始化也有类似的问题~

字段初始化目前是在哪里有什么问题么,暂时还没发现,目前就发现 nil 进去了出来就不是 nil 了

记得有一些 preload 应该是这样子的,或者嵌套字段

大佬,其实我一直有点疑惑,not found 这种情况严格意义来说,是一种错误么,之前在写用 mybatis 的时候,记得行为应该就是找不到就是 null,not found 放在 error 里面,总是会让代码写的变成这样


`result ,err := find()
if err != nil && !errors.Is(err, gorm.ErrNotFound){
    panic(err)
}
if errors.Is(err, gorm.ErrNotFound){
  // do 
}

代码看起来不是很优雅,一般的业务程序很多应该都是需要手动去处理 not found 吧,如果变成下面这样,会不会更友好呢「我们目前的业务场景下面看来,not found 往往是一个程序可以处理到的 error」

result ,err := find()
if err != nil {
    panic(err)
}
// 这里如果 result 原本就不是指针,可能是去判断 result 的某个值
if result == nil {
  // do 
}

@jinzhu
Copy link
Member

jinzhu commented Apr 15, 2024

First,Last 这种是期望找到数据的,所以找不到会返回错误,如果不期望有错误发生可以 Find + Limit 方法

@lcgash
Copy link
Author

lcgash commented Apr 15, 2024

First,Last 这种是期望找到数据的,所以找不到会返回错误,如果不期望有错误发生可以 Find + Limit 方法

懂了,感谢感谢

@lcgash lcgash closed this Apr 15, 2024
@lcgash
Copy link
Author

lcgash commented Apr 15, 2024

感觉这个修改没有啥必要,因为字段的初始化也有类似的问题~

还是这个问题,jinzhu 大佬,是不是如果调整下顺序变为
db.RowsAffected > 0 再去做 dest 的数据初始化,会更好点,这样能保持用户传入进去的,遇到没有任何数据变更的情况下,不会发生任何改变呢

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.

None yet

3 participants