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

[CHANGED] Hide deleted objects by default #1091

Merged
merged 1 commit into from
Sep 21, 2022
Merged

Conversation

piotrpio
Copy link
Collaborator

Addresses nats-io/nats-architecture-and-design#155

This PR introduces breaking changes to object store method signatures, as object store is still in experimental preview state.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 85.791% when pulling 08dd7d3 on hide-deleted-objects into 8e72eaa on main.

@@ -60,25 +60,25 @@ type ObjectStore interface {
// Put will place the contents from the reader into a new object.
Put(obj *ObjectMeta, reader io.Reader, opts ...ObjectOpt) (*ObjectInfo, error)
// Get will pull the named object from the object store.
Get(name string, opts ...ObjectOpt) (ObjectResult, error)
Get(name string, opts ...GetObjectOpt) (ObjectResult, error)
Copy link
Member

Choose a reason for hiding this comment

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

Although we can break since it was indeed marked experimental, do we have to? Is the idea here to logically separate get options vs put or some others? You could still have the internal objOpts have sub-structs for individual type of options: put, get, getinfo, etc... but still have options that are ObjectOpt, and "get" would just use the "get" options, etc...
But I agree that it would not prevent an user to pass say a "put" option to a "get" API, so in that sense it is better to have specific options, even if under the cover they could feed into a single internal option struct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that is the idea. It is not a great user experience to be able to pass options to methods which make no sense. I think having it as separate type per method or group of similar methods makes things easier to the user (no guessing about which options apply to which methods).

Functional options are tricky either way, as you usually don't have the IDE support to e.g. easily find all options applying to a method, plus there is a possibility of options collisions, so I try to make it less complicated when possible.

Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

If the options signatures are the same, meaning working code will still work and we only disallow what we thought was incorrect behavior I am ok with it.

@piotrpio
Copy link
Collaborator Author

TL;DR In GetInfo() and List() IgnoreDeletes() will not compile, but that's intentional as we want to hide deletes by default: nats-io/nats-architecture-and-design#155

@derekcollison so for each method for which ...ObjectOpt changed to ...GetObjectOpt it will work just fine, as there used to be only one option (nats.Context()) anyway, and that will work the same.

With GetInfo() and List() it is a bit different and it might require code changes as the option type changes from WatchOpt to GetObjectInfoOpt and ListObjectsOpt respectively.
So, in those 2 methods you won't be able to useoptions which did not make sense anyway (IncludeHistory() and MetaOnly() - they are not used anywhere in object store). Also you will not be able to use IgnoreDeletes(), but that is the point of this PR - to change the behavior and by default not show deleted objects, only enable them with option (GetObjectInfoShowDeleted())

@derekcollison
Copy link
Member

We could allow IgnoreDeletes() to still compile right? That is easy to do.

@piotrpio
Copy link
Collaborator Author

Not really easy to do as IgnoreDeletes() returns a WatchOpt and I've changed the option types to be more tightly coupled to the methods they are used with.
We could get rid of the new types and just add ShowDeletes() as WatchOpt, but that is what I'm trying hard not to do - not to add options which only make sense in specific methods.

I know that it is a breaking change but fixing it in user's code is extremely simple (just remove IgnoreDeletes() from the call) so for experimental preview it should be all right.

Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

@piotrpio piotrpio merged commit f410242 into main Sep 21, 2022
@piotrpio piotrpio deleted the hide-deleted-objects branch September 21, 2022 18:08
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.

4 participants