[#121] client: Make PrmObjectGet/Head/GetRange fields public #153

aarifullin · 2023-08-28T08:12:23Z

aarifullin commented

2023-08-28 08:12:23 +00:00

Remove common PrmObjectRead structure
Introduce buildRequest for PrmObjectGet/Head/GetRange
Refactor the usage of these params in pool

Signed-off-by: Airat Arifullin a.arifullin@yadro.com

* Remove common PrmObjectRead structure * Introduce buildRequest for PrmObjectGet/Head/GetRange * Refactor the usage of these params in pool Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>

aarifullin added the

refactoring

label 2023-08-28 08:12:23 +00:00

aarifullin requested review from storage-core-committers 2023-08-28 08:13:24 +00:00

aarifullin requested review from storage-core-developers 2023-08-28 08:13:25 +00:00

aarifullin requested review from storage-services-committers 2023-08-28 08:13:45 +00:00

aarifullin requested review from storage-services-developers 2023-08-28 08:13:45 +00:00

aarifullin force-pushed feature/121-prm_object_get from 10884a7d11 to 84e7e69f98

2023-08-28 08:27:08 +00:00

Compare

dkirillov reviewed 2023-08-28 08:52:22 +00:00

client/object_get.go

					
				@ -90,4 +25,2 @@

				// PrmObjectGet groups parameters of ObjectGetInit operation.

				type PrmObjectGet struct {

					prmObjectRead

dkirillov commented

2023-08-28 08:52:22 +00:00

It breaks backward compatibility. Is it ok?

aarifullin commented

2023-08-28 09:25:45 +00:00

I can agree that this refactoring looks a little bit aggressive, but it depends on the point how we consider supporting the backward compatibility.
I suppose you're talking about using the setters

    if prm.sessionToken != nil {
		getPrm.WithinSession(*prm.sessionToken)
	}

	if prm.bearerToken != nil {
		getPrm.WithBearerToken(*prm.bearerToken)
	}

	if prm.raw {
		getPrm.MarkRaw()
	}

	if prm.local {
		getPrm.MarkLocal()
	}

From the point of the usage in frostfs-node such refactoring won't be painful.

Another option it is to make:

type PrmObjectGet struct {
    PrmObjectRead
}

that means that from the client side I need to construct PrmObjectGet like that:

prm := PrmObjectGet{
   PrmObjectRead: PrmObjectRead{
   }, 
   ...
}

I don't like this idea to gather common fields in one struct and then impose the such constructor because of specific implementation

I can agree that this refactoring looks a little bit aggressive, but it depends on the point how we consider supporting the backward compatibility. I suppose you're talking about using the setters ```golang if prm.sessionToken != nil { getPrm.WithinSession(*prm.sessionToken) } if prm.bearerToken != nil { getPrm.WithBearerToken(*prm.bearerToken) } if prm.raw { getPrm.MarkRaw() } if prm.local { getPrm.MarkLocal() } ``` From the point of the usage in `frostfs-node` such refactoring won't be painful. Another option it is to make: ```golang type PrmObjectGet struct { PrmObjectRead } ``` that means that from the client side I need to construct `PrmObjectGet` like that: ```golang prm := PrmObjectGet{ PrmObjectRead: PrmObjectRead{ }, ... } ``` I don't like this idea to gather common fields in one struct and then impose the such constructor because of specific implementation

dkirillov commented

2023-08-28 11:28:08 +00:00

I just wanted to highlight that the client code won't compile without changes if we update the SDK this way. if we do it consciously then ok

aarifullin commented

2023-08-28 12:56:21 +00:00

Sure, the client must be updated

fyrchik commented

2023-08-28 13:21:04 +00:00

Can we retain all the methods with similar Deprecated: warning?
Not that it matters a lot, but it also seems not painful to do.

Can we retain all the methods with similar `Deprecated:` warning? Not that it matters a lot, but it also seems not painful to do.

aarifullin commented

2023-08-28 14:05:20 +00:00

Not that it matters a lot, but it also seems not painful to do.

I have removed prmObjectRead out from PrmObjectGet/Head/GetRange and thus these methods do not inherit the setters. If we want to keep these methods, I need to define "deprecated" method for these three structs.
Also, some types have been reworked because it was:

type prmObjectRead struct {
	meta v2session.RequestMetaHeader

	raw bool

	addr v2refs.Address
}

FromContainer and ByID were used to set addr (api-type). As I said, I think parameters should keep only sdk types but not api (that is way cid.ID, oid.ID are introduced). So, we cannot keep these setters

> Not that it matters a lot, but it also seems not painful to do. I have removed `prmObjectRead` out from `PrmObjectGet/Head/GetRange` and thus these methods do not inherit the setters. If we want to keep these methods, I need to define "deprecated" method for these three structs. Also, some types have been reworked because it was: ```golang type prmObjectRead struct { meta v2session.RequestMetaHeader raw bool addr v2refs.Address } ``` `FromContainer` and `ByID` were used to set `addr` (api-type). As I said, I think parameters should keep only sdk types but not api (that is way `cid.ID, oid.ID` are introduced). So, we cannot keep these setters

👍 1

dkirillov marked this conversation as resolved

dstepanov-yadro reviewed 2023-08-28 13:29:46 +00:00

client/object_get.go

					
				@ -490,0 +545,4 @@

						return nil, errorMissingObject

					}

					if len(prm.XHeaders)%2 != 0 {

dstepanov-yadro commented

2023-08-28 13:29:18 +00:00

I think length/offset check is also required.

aarifullin commented

2023-08-28 13:45:24 +00:00

Length check already exists (see above)

if prm.Length == 0 {
   return nil, errorZeroRangeLength
}

Do we need to check the offset field? I mean, what should we check for the offset?

Length check already exists (see above) ```golang if prm.Length == 0 { return nil, errorZeroRangeLength } ``` Do we need to check the offset field? I mean, what should we check for the offset?

dstepanov-yadro commented

2023-08-28 14:03:50 +00:00

Ups, my bad

dstepanov-yadro marked this conversation as resolved

dstepanov-yadro approved these changes 2023-08-28 14:03:54 +00:00

dkirillov approved these changes 2023-08-28 15:00:33 +00:00

acid-ant approved these changes 2023-08-29 06:12:18 +00:00

dstepanov-yadro approved these changes 2023-08-29 07:07:27 +00:00

fyrchik merged commit 84e7e69f98 into master

2023-08-29 07:23:35 +00:00

Sign in to join this conversation.

No reviewers