Skip to content
This repository has been archived by the owner on Jan 24, 2021. It is now read-only.

Add support for .net 4.5 data annotations #2435

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add support for .net 4.5 data annotations #2435

wants to merge 5 commits into from

Conversation

xt0rted
Copy link
Contributor

@xt0rted xt0rted commented Apr 30, 2016

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified made sure that I am following the Nancy code style guidelines
  • I have provided test coverage for your change (where applicable)

Description

This is an almost exact copy/paste of xt0rted/Nancy.Validation.DataAnnotations.Extensions. I updated the coding style in some areas, but didn't carry over what was in the sample project.

I also removed a prior mono hack since we're targeting versions which fix the bug we were working around.

The tests I carried over used a base class so I didn't duplicate the setup code. If you'd like that removed say the word.

This fixes #1577

@xt0rted
Copy link
Contributor Author

xt0rted commented Apr 30, 2016

Looks like the mono bug wasn't fixed like I thought. For now I'm going to back out that commit.

@xt0rted
Copy link
Contributor Author

xt0rted commented Apr 30, 2016

I take that back, the mono bug was fixed. This is something new that's not the same between implementations.

@xt0rted
Copy link
Contributor Author

xt0rted commented Apr 30, 2016

Mono uses Microsoft/referencesource for their implementation, but Microsoft never published the resx file to here. Because of that the resource names are being returned instead of the actual resource string.

https://github.com/dotnet/corefx/issues/8201 reporting the inconsistency

@xt0rted
Copy link
Contributor Author

xt0rted commented May 3, 2016

@thecodejunkie or @khellang based on the milestone added to my corefx issue the resource strings won't make it into an official release until November 2016, plus however long to make it into mono.

Should I just turn those tests off on mono for now, or would it be better to update the tests to check if the result is the expected message or the resource name instead of just the message? Either would get this to pass on mono.

@horsdal
Copy link
Member

horsdal commented Aug 4, 2016

I think they should just be turned off (like already did, right?), since this is essentially hitting a mono bug.

Also: The PR needs a rebase :)

Otherwise it looks good.

@xt0rted
Copy link
Contributor Author

xt0rted commented Aug 4, 2016

The teamcity build is failing because the project is referencing the AssemblyInfo.cs which isn't in the project. Is that correct? If so I'll remove the reference.

I see, we're using SharedAssemblyInfo.cs now. I'll fix that after dinner.

@jchannon
Copy link
Member

jchannon commented Aug 5, 2016

Yup it's gone

On Friday, 5 August 2016, Brian Surowiec [email protected] wrote:

The teamcity build is failing because the project is referencing the
AssemblyInfo.cs which isn't in the project. Is that correct? If so I'll
remove the reference.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#2435 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGaph0zA_6WuTxGs_hJpxaqWA26efR8ks5qcnPMgaJpZM4ITai5
.

@xt0rted
Copy link
Contributor Author

xt0rted commented Aug 5, 2016

BrowserFixture.cs#L441-L461 that's passing locally but failed on TeamCity

@jchannon
Copy link
Member

jchannon commented Aug 5, 2016

I know. No idea why

On Friday, 5 August 2016, Brian Surowiec [email protected] wrote:

BrowserFixture.cs#L441-L461

[Fact]
public async Task Should_return_JSON_serialized_form()
{
// Given
var response = await browser.Post("/serializedform", (with) =>
{
with.HttpRequest();
with.Accept("application/json");
with.FormValue("SomeString", "Hi");
with.FormValue("SomeInt", "1");
with.FormValue("SomeBoolean", "true");
});
// When
var actualModel = response.Body.DeserializeJson<EchoModel>();
// Then
Assert.Equal("Hi", actualModel.SomeString);
Assert.Equal(1, actualModel.SomeInt);
Assert.Equal(true, actualModel.SomeBoolean);
}

that's passing locally but failed on TeamCity


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#2435 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGapmH6oTUbWadNfRa2CuhqzDsdme3Nks5qcvcJgaJpZM4ITai5
.

@jchannon
Copy link
Member

jchannon commented Aug 5, 2016

There's another issue or PR that suggests it's this

#2509

On Friday, 5 August 2016, Jonathan Channon [email protected]
wrote:

I know. No idea why

On Friday, 5 August 2016, Brian Surowiec <[email protected]
javascript:_e(%7B%7D,'cvml','[email protected]');> wrote:

BrowserFixture.cs#L441-L461

[Fact]
public async Task Should_return_JSON_serialized_form()
{
// Given
var response = await browser.Post("/serializedform", (with) =>
{
with.HttpRequest();
with.Accept("application/json");
with.FormValue("SomeString", "Hi");
with.FormValue("SomeInt", "1");
with.FormValue("SomeBoolean", "true");
});
// When
var actualModel = response.Body.DeserializeJson<EchoModel>();
// Then
Assert.Equal("Hi", actualModel.SomeString);
Assert.Equal(1, actualModel.SomeInt);
Assert.Equal(true, actualModel.SomeBoolean);
}

that's passing locally but failed on TeamCity


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#2435 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGapmH6oTUbWadNfRa2CuhqzDsdme3Nks5qcvcJgaJpZM4ITai5
.

@xt0rted xt0rted mentioned this pull request Sep 28, 2016
4 tasks
@xt0rted
Copy link
Contributor Author

xt0rted commented Sep 28, 2016

Travis failed because of this test which isn't even related Nancy.Authentication.Forms.Tests.FormsAuthenticationFixture.Should_retain_querystring_when_redirecting_after_successfull_login

TeamCity is failing because of #2578

@xt0rted
Copy link
Contributor Author

xt0rted commented Nov 21, 2016

There was an adapter missing for EnumDataTypeAttribute so instead of sending in another PR I just made it part of this one.

@xt0rted
Copy link
Contributor Author

xt0rted commented Feb 13, 2017

Not sure if anyone has been following https://github.com/dotnet/corefx/issues/8201 but it seems like there's a long ways to go before mono will get proper resource strings for data annotations. They're even holding off on using CoreFX because of feature parity. Guess we'll be keeping these tests turned off for the foreseeable future.

Is there anything holding this PR up now? I've tried to keep on top of rebasing when master changes but I'm not sure if there's anything preventing it from being merged now aside from the disabled tests on mono.

These are disabled for now because of mono's implementation missing the
resource strings. Once Microsoft fixes this in the reference source
code, and mono pulls it in, then we'll be able to turn these back on.
@xt0rted
Copy link
Contributor Author

xt0rted commented Jun 27, 2017

Rebased and ready for review again.

The new changes since this was last looked at are adding support for EnumDataTypeAttribute and adding the validation sample project back in (they were all removed during project.json).

Once this is pulled in I'd like to send in another PR for Fluent Validation that adds support for v7 (the sample project depends on it which is why I'm waiting). Do we want to talk about those changes first or just send in the changes and we can take it from there?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for .net 4.5 data annotations
3 participants