Nancy.Swagger icon indicating copy to clipboard operation
Nancy.Swagger copied to clipboard

ObjectSchema member.Key.ToCamelCase() ignores underscores

Open sberney opened this issue 5 years ago • 5 comments

Expected Behavior

I have class like this:

public class MyClass
{
  [JsonIgnore]
  [ModelProperty(Ignore = true)]
  public string _isSeven { get; set; }

  public bool IsSeven
  {
    get
    {
      return _isSeven == 'True';
    }

    set
    {
      _isSeven = value ? 'True' : 'False';
    }
  }
}

I expect including this class in the ModelCatalog to not totally bork the Nancy.Swagger /api-docs route. I also expect it not to add _isSeven into the Schema Properties, or at least to do so under a different name.

Taking a look at the relevant code, the object properties are normalized using member.Key.ToCamelCase(), which maps both IsSeven and _isSeven to the same string, isSeven.

While I understand (and enjoy) normalization to camelCase, in the project I am working on, it is the convention to name sometimes name an underlying property the same as the public property, but prefixing the name with an underscore, eg IsSeven and _isSeven to be ultra pedantic. At the very least, the [ModelProperty(Ignore = true)] ought to fix this behavior (the error message for which is obtuse), or another property.

I hesitate to introduce Nancy.Swagger to my colleagues when I know that they will eventually have their swagger docs broken after trying to include a particular model.

Steps to Reproduce the Problem

  1. Open Nancy.Swagger sample annotations project
  2. Add class as described above somewhere in the project.
  3. Add class to ModelCatalog.
  4. Attempt to access generated docs -- will not work

Specifications

  • Version: 2.2.53-alpha through and including master
  • Project: Nancy.Swagger
  • Platform: Windows, .NET 4.6.1; probably all

Thank you, Sam Berney

sberney avatar Feb 25 '19 23:02 sberney

Does this still happen if you make _isSeven private? From all the conventions I've ever used, _variableName indicated a private member, which I believe would be excluded from this schema generation. I believe that's why it wasn't originally considered.

If you change it from a property to a member variable public string _isSeven;, then you would also resolve this issue.

But you're right, the Ignore = true should at least work. I'll check that out now.

jnallard avatar Feb 26 '19 02:02 jnallard

The code is working as expected in terms of Ignore = true. The problem is that you haven't specified the class as a Model via the ModelAttribute, so it's not being modeled via attributes, but via the default modeling from the non-annotations project.

You can fix this by changing the class to be like:

[Model("This is my class")]
public class MyClass
{
	[JsonIgnore]
	[ModelProperty(Ignore = true)]
	public string _isSeven { get; set; }

	public bool IsSeven
	{
		get
		{
			return _isSeven == "True";
		}

		set
		{
			_isSeven = value ? "True" : "False";
		}
	}
}

Let us know if that resolves your error!

jnallard avatar Feb 26 '19 02:02 jnallard

Thanks, @jnallard,

I was forgetting to do that! But, while adding that hides the annotated properties from the Model schema, it doesn't prevent exceptions from being thrown for similarly named variables.

Still, when I access /api-docs, an exception is thrown even after adding the [Model("abcde")] annotation to the class. It doesn't happen if name the property _isSeven something else, like _isSevenInternal. I'd like to avoid having to rename everything in the long run.

Here is a stacktrace:

Nancy.RequestExecutionException: Oh noes! ---< System.ArgumentException: An item with the same key has already been added.
   at System.ThrowHelper.ThrowArgumentException(ExceptionResource resource)
   at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
   at Nancy.Swagger.SwaggerSchemaFactory.ObjectSchema..ctor(Type t, Model sModel, Boolean isDefinition)
   at Nancy.Swagger.SwaggerSchemaFactory.CreateSchema(Model sModel, Type t, Boolean isDefinition)
   at Nancy.Swagger.Services.SwaggerMetadataProvider.GetSwaggerJson(NancyContext context)
   at Nancy.Swagger.Modules.SwaggerModule.><c__DisplayClass0_0.>.ctor<b__0(Object _)
   at Nancy.NancyModule.><c__DisplayClass14_0`1.>Get<b__0(Object args)
   at Nancy.NancyModule.><c__DisplayClass16_0`1.>Get<b__0(Object args, CancellationToken ct)
   at Nancy.Routing.Route`1.Invoke(DynamicDictionary parameters, CancellationToken cancellationToken)
   at Nancy.Routing.DefaultRouteInvoker.>Invoke<d__2.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Nancy.Routing.DefaultRequestDispatcher.>Dispatch<d__5.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Nancy.NancyEngine.>InvokeRequestLifeCycle<d__22.MoveNext()
   --- End of inner exception stack trace ---
   at Nancy.NancyEngine.InvokeOnErrorHook(NancyContext context, ErrorPipeline pipeline, Exception ex)

Thank you

sberney avatar Feb 27 '19 00:02 sberney

Hmm. I did test out your example and it did resolve the error for me. Are you sure you added the model property to all the corresponding classes?

Otherwise, if you want to make a pr to keep the starting _, we can review it later. I'd still prefer to get the Ignore = true working though.

jnallard avatar Feb 27 '19 00:02 jnallard

Ok, I do admit I didn't open up the sample project before replying that it doesn't work -- I did it directly in my own project. I'll spend a little more time on it. Thanks!

sberney avatar Feb 27 '19 00:02 sberney