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

Add the ability to specify a comparator for array.unique() for objects only #860

Closed
wants to merge 1 commit into from
Closed

Conversation

DavidTPate
Copy link
Contributor

I was looking at #859 and thought that it seemed like something that belonged in the core of Joi, not as a plugin/extension and had a pretty good idea of how I would implement it. This PR contains that initial implementation.

What It Does

It allows a user to specify a comparator function which is used only for objects.

// Compare objects with custom logic
const objectComparator = (left, right) => {

  return left.username.toLowerCase() = right.username.toLowerCase();
};

const objectSchema = Joi.array().unique({
  objectComparator: objectComparator
});

Joi.attempt([{ username: 'bob' }, { username: 'BoB' }], objectSchema); // throws error
const result = Joi.attempt([{ username: 'bob', name: 'Bob' }, { username: 'bob-2', name: 'Bob' }], objectSchema); // result -> [{ username: 'bob' }, { username: 'bob-2' }]

In the event that a comparator isn't provided it just defaults to the standard behavior using Hoek.deepEqual() so the change has a default and also is backwards compatible.

What It Doesn't Do

It doesn't allow a comparator to be provided for types other than object. Originally I wanted to make the parameter just comparator and use it for everything, but once I started implementing this I realized I couldn't do that without a noticeable decrease in performance because of how it works right now using an object map.

The only cases I can think of where a comparator function would be useful for things other than objects is when the data needs to be modified, which that could just be done prior to validation. For example, if an array of numbers needs to have Math.floor() run on each value and in that case I imagine you would want the value to come out of validation already run through Math.floor() anyways. Others would be string manipulation and such like lowercase comparisons, etc.

…paring objects only. This doesn't include comparing other types with a comparator becuase it would result in a performance decrease based upon how the comparison currently occurs.
@Marsup Marsup added this to the 8.1.0 milestone Apr 9, 2016
@Marsup Marsup added the feature New functionality or improvement label Apr 9, 2016
@Marsup Marsup self-assigned this Apr 9, 2016
@Marsup
Copy link
Collaborator

Marsup commented Apr 9, 2016

I don't see why it would only compare objects. Also a simple function should be enough.

@Marsup
Copy link
Collaborator

Marsup commented May 3, 2016

@DavidTPate thoughts about my last comment ?

@DavidTPate
Copy link
Contributor Author

@Marsup Sorry, been out-of-pocket for a bit. The reason why I only did it for objects was because I would need to change how the compares work for non-objects pretty considerably and also reduce the current performance quite a bit (at least that's what I thought at the time).

Looking at this again, I think I was just being a bit of an idiot. I'll go through and fix some of those references as it should be that this only compares Arrays which is done in this block while non-Arrays are checked in this block. The second block I would need to reduce the performance of in order to support this functionality since right now it uses an Object Map to keep track of uniqueness based on type.

I didn't do the less performant part because I expected it would be rejected, but I'll put that together so we can see what the whole change set would be and determine the action from there.

@Marsup Marsup modified the milestones: 8.2.0, 8.1.0 May 7, 2016
@Marsup Marsup modified the milestones: 8.3.0, 8.2.0 May 28, 2016
@Marsup Marsup added this to the 9.0.0 milestone Jun 22, 2016
@Marsup
Copy link
Collaborator

Marsup commented Jun 22, 2016

Borrowed some of your code in 038c8ff, thanks !

@Marsup Marsup closed this Jun 22, 2016
@DavidTPate DavidTPate deleted the issue-859 branch June 23, 2016 03:10
@DavidTPate
Copy link
Contributor Author

Sounds good, thanks @Marsup It's been crazy for me here.

@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New functionality or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants