Skip to content

Add ability to configure id properties in code #1145

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

Merged
merged 1 commit into from
Dec 15, 2014

Conversation

gmarz
Copy link
Contributor

@gmarz gmarz commented Dec 14, 2014

Previously, the only way to specify an id property was to use the ElasticTypeAttribute:

[ElasticType(IdProperty="AlternateId"]

This PR introduces an alternative way to configure id properties, via code:

var settings = new ConnectionSettings()
    .MapIdPropertyFor<Foo>(o => o.AlternateId);

Note: Mapping two different id properties on the same type is an error:

var settings = new ConnectionSettings()
    .MapIdPropertyFor<Foo>(o => o.AlternateId)
    .MapIdPropertyFor<Foo>(o => o.AnotherId);

and will cause an ArgumentException to be thrown.

Question: How should we treat the case where a type has an Id property set via ElasticTypeAttribute and an id property is mapped in code? Currently, the code mapping will take precedence, but I wonder if this should be an error instead?

Closes #407

@Mpdreamz
Copy link
Member

LGTM!

I do not think it should be an error and the current precedence makes sense. In the esoteric case where a type is marked with an ElasticTypeAttribtute and through a ConnectionSettings mapping. The ConnectionSettings mapping is the most "local" and the only way to override an already marked type (possible in a compiled assembly).

Mpdreamz added a commit that referenced this pull request Dec 15, 2014
Add ability to configure id properties in code
@Mpdreamz Mpdreamz merged commit 15fee30 into develop Dec 15, 2014
@Mpdreamz Mpdreamz deleted the feature/idproperty-in-code branch December 15, 2014 09:44
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