WIP: Bind to stream rule #1387

MikeStall · 2017-10-16T18:25:03Z

Add a new BindToStream rule.

Resolves: #1001

This includes tests, and is verified for OneDrive. But does not yet plumb Blob on top of this.

ConnorMcMahon · 2017-10-17T18:01:55Z

src/Microsoft.Azure.WebJobs.Host/Bindings/BindingProviders/BindToStreamBindingProvider.cs

+            {
+                yield return new BindingRule
+                {
+                    SourceAttribute = typeof(BlobAttribute),


Should BlobAttribute be more general here? #Resolved

ConnorMcMahon · 2017-10-17T18:14:48Z

src/Microsoft.Azure.WebJobs.Host/Bindings/BindingProviders/BindToStreamBindingProvider.cs

+        }
+
+
+        private static FileAccess? GetFileAccessFromAttribute(Attribute attribute)


This logic could be a lot cleaner if we had the guarantee that all binding Attributes that wanted to have a BindToStream rule implemented some interface along the lines of IStreamableBindingAttribute, that had a FileAccess? property guaranteed to be present.

This comes with a few downsides though, such as retrofitting older attributes to require this interface (possibly breaking changes?) and adding an extra knowledge burden on extension developers. However, avoiding reflection would be nice, and it does make sense semantically as well.

ConnorMcMahon · 2017-10-17T18:17:33Z

src/Microsoft.Azure.WebJobs.Host/Bindings/BindingProviders/BindToStreamBindingProvider.cs

+                        break;
+
+                    default:
+                        throw new NotImplementedException("ReadWrite access is not supported. Pick either Read or Write.");


Is there a reason we don't support ReadWrite access? It may make sense for future bindings, even if it doesn't for our current ones. #Resolved

Our current streams are single-direction; that also greatly simplifies any marshalling.
We can always add bi-directional support later.

In reply to: 145212138 [](ancestors = 145212138)

ConnorMcMahon · 2017-10-17T18:29:05Z

src/Microsoft.Azure.WebJobs.Host/Bindings/BindingProviders/BindToStreamBindingProvider.cs

+            }
+
+            VerifyOrThrow(actualAccess, isRead);
+            if (!IsSupportedByRule(isRead))


If I understand this logic, we have 3 different sources of FileAccess:

What the rule has specified (Read, Write, or ReadWrite)

What the attribute specifies (Read or Write)

What the parameter type supports (Read or Write).

Is there a reason that if 1 and 3 disagree (with 1 = ReadWrite agreeing with any value for 3), we return null, but if 2 and 3 disagree we throw an exception? #ByDesign

Yes. Because if the rule only supports Read, then it's possible there's another rule that supports Write. IE, we can't assert it's an error. Whereas a conflict between #2 + #3 is definitely an error.

In reply to: 145215473 [](ancestors = 145215473)

ConnorMcMahon · 2017-10-17T18:49:10Z

src/Microsoft.Azure.WebJobs.Host/Bindings/BindingProviders/BindToStreamBindingProvider.cs

+                return _stream;
+            }
+
+            public async Task InitAsync(Func<Stream> builder, Type userType)


Consider allowing async conversions of TAttribute to Stream, and making GetOrCreateStream() an async method, as all of its callers are async as well. #Resolved

I believe we do support async. I called out an example in the tests.

In reply to: 145221230 [](ancestors = 145221230)

I took a look at patternmatcher.TryGetConverterFunc() and it looks like it calls CreateConverterFunc, which uses reflection and invokers to translate the async Func<TAttribute, Task> to a synchronous Func<TAttribute, BindType>.

In reply to: 145235436 [](ancestors = 145235436,145221230)

That would still be a separate fix in the core of the SDK. The BindToInputRule and Extension are async. #Resolved

ConnorMcMahon · 2017-10-17T18:57:38Z

test/Microsoft.Azure.WebJobs.Host.UnitTests/Common/BindToGenericItemTests.cs

@@ -11,6 +11,9 @@
 using System.Threading;
 using Newtonsoft.Json;
 using Microsoft.Azure.WebJobs.Description;
+using System.IO;


Are these actually required? #Resolved

ConnorMcMahon · 2017-10-17T19:04:08Z

test/Microsoft.Azure.WebJobs.Host.UnitTests/Common/BindToStreamTests.cs

+                throw new InvalidOperationException();
+            }
+
+            // Explicit Write access 


Is this comment correct? #Resolved

ConnorMcMahon · 2017-10-17T19:05:01Z

test/Microsoft.Azure.WebJobs.Host.UnitTests/Common/BindToStreamTests.cs

+
+            // Explicit Write access 
+            public void Read(
+                [TestStream(Path="{x}-%y%")] string value


Where does the %y% come from? #Resolved

%...% means "take from appsettings". The test wired up an appsetting converter to substitute this.

In reply to: 145225541 [](ancestors = 145225541)

ConnorMcMahon · 2017-10-17T19:15:15Z

test/Microsoft.Azure.WebJobs.Host.UnitTests/Common/BindToStreamTests.cs

+
+        // Bulk test the success case for different parameter types we can bind to. 
+        [Fact]
+        public void TestStream()


In general I don't like to have so many test cases covered by one test, but I don't see a clear way to split up the tests without adding far more classes. #Resolved

Exactly :| There's so much machinery to just wire up the extension for testing; it'd be painful to repeat it or every case.

In reply to: 145228086 [](ancestors = 145228086)

MikeStall · 2017-10-17T19:40:19Z

test/Microsoft.Azure.WebJobs.Host.UnitTests/Common/BindToStreamTests.cs

+
+            #endregion // #region Write overloads 
+
+            public async Task<Stream> ConvertAsync(TestStreamAttribute input, CancellationToken cancellationToken)


public async Task ConvertAsync(TestStreamAttribute input [](start = 12, length = 64)

@ConnorMcMahon - here's an example of an AsyncConverter that you asked about. #Resolved

mathewc · 2017-10-18T14:34:29Z

src/Microsoft.Azure.WebJobs.Host/Bindings/BindingProviders/BindToStreamBindingProvider.cs

+            }
+        }
+
+        private void VerifyOrThrow(FileAccess? actualAccess, bool isRead)


nit: VerifyAccess? Also "actualAccess" isn't clear. Maybe "declaredAccess" since it's the attribute declared value? #Resolved

mathewc · 2017-10-18T14:43:56Z

src/Microsoft.Azure.WebJobs.Host/Bindings/BindingProviders/BindToStreamBindingProvider.cs

+                {
+                    if (!CanRead(actualAccess.Value))
+                    {
+                        errorMsg = "Read";


Can you just ToString the enum value to get the string, rather than forming these strings manually? Also, rather than passing in an "isRead" bool, why not just pass in a second FileAccess? You'd then have VerifyAccess(FileAccess? declaredAccess, FileAccess desiredAccess)

mathewc · 2017-10-18T14:46:08Z

src/Microsoft.Azure.WebJobs.Host/Bindings/BindingProviders/BindToStreamBindingProvider.cs

+            }
+
+            var parameter = context.Parameter;
+            var typeUser = parameter.ParameterType;


nit: "typeUser" isn't really clear. Why not just "parameterType"? #Resolved

mathewc · 2017-10-18T14:47:13Z

src/Microsoft.Azure.WebJobs.Host/Bindings/BindingProviders/BindToStreamBindingProvider.cs

+                {
+                    case FileAccess.Read:
+                        isRead = true;
+


nit: formatting is off throughout this switch it seems #WontFix

I'm just using VS's auto-format.

In reply to: 145437932 [](ancestors = 145437932)

mathewc · 2017-10-18T14:54:46Z

src/Microsoft.Azure.WebJobs.Host/Bindings/BindingProviders/BindToStreamBindingProvider.cs

+        {
+            var prop = attribute.GetType().GetProperty("Access", BindingFlags.Public | BindingFlags.Instance);
+
+            bool ok = (prop != null);


I think your code would be clearer w/o this unecessary "ok" local. Can't you just use "prop" and set it to null if required? #Resolved

mathewc · 2017-10-18T15:03:10Z

src/Microsoft.Azure.WebJobs.Host/Bindings/BindingProviders/BindToStreamBindingProvider.cs

+            // Helper to build the stream. This will only get invoked once and then cached as _stream. 
+            private Func<Stream> _streamBuilder;
+
+            object _userArg;


can you clean up these declarations (order properly and remove all the unecessary spacing)? Also, I wonder if fxcop is running - should complain that you don't have "private" access modifier. #Resolved

mathewc · 2017-10-18T15:04:00Z

src/Microsoft.Azure.WebJobs.Host/Bindings/BindingProviders/BindToStreamBindingProvider.cs

+            public async Task InitAsync(Func<Stream> builder, Type userType)
+            {
+                Type = userType;
+                _invokeString = "???";


What is the right value to use here?

mathewc · 2017-10-18T15:04:48Z

src/Microsoft.Azure.WebJobs.Host/Bindings/BindingProviders/BindToStreamBindingProvider.cs

+            abstract protected Task<object> CreateUserArgAsync();
+
+            // Give derived object a chance to flush any buffering before we close the stream. 
+            virtual protected Task FlushAsync() { return Task.CompletedTask;  }


nit: protected virtual #Resolved

mathewc · 2017-10-18T15:08:12Z

src/Microsoft.Azure.WebJobs.Host/Bindings/BindingProviders/BindToStreamBindingProvider.cs

+                _invokeString = "???";
+                _streamBuilder = builder;
+
+                _userArg = await this.CreateUserArgAsync();


The "user arg" naming isn't clear. Can you rename things to _value = await GetValueAsync();? #Resolved

I'll do _userValue. by 'user', I mean value passed to the user's function (as opposed to the internal values we track)

In reply to: 145444786 [](ancestors = 145444786)

mathewc · 2017-10-18T15:11:18Z

src/Microsoft.Azure.WebJobs.Host/Extensions/JobHostMetadataProvider.cs


-                if (metadata.TryGetValue("direction", StringComparison.OrdinalIgnoreCase, out token))
+            // Other "look like a stream" attributes may expose an Access property for stream direction. 
+            var prop = attributeType.GetProperty("Access", BindingFlags.Instance | BindingFlags.Public);


Can we use a shared helper for this in all the 3 places I've seen this? #Resolved

Includes Script / Function.json support. verified plumbing [Blob] on top of this. Breaking changes from Blob: - When using ICloudBinderSupport<T>, when stream is missing, custom converter is not called. We just use the default value. - BOM - 'out string' does not emit a bom. Be consistent with TextWriter. - When binding to a Write Stream, 0-byte stream is still created if you write 0 bytes. Before, [Blob] would avoid creating stream.

MikeStall · 2017-11-08T18:16:57Z

CR feedback is incorporated and this has been superseded by #1422

dnfclas added the cla-already-signed label Oct 16, 2017

MikeStall mentioned this pull request Oct 16, 2017

Plumb [File] on SDK's new BindToInput rule. Azure/azure-webjobs-sdk-extensions#314

Closed

ConnorMcMahon reviewed Oct 17, 2017

View reviewed changes

MikeStall commented Oct 17, 2017

View reviewed changes

mathewc requested changes Oct 18, 2017

View reviewed changes

MikeStall force-pushed the BindToStream branch from c24f1a9 to b45ecd0 Compare October 24, 2017 23:18

MikeStall closed this Nov 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Bind to stream rule #1387

WIP: Bind to stream rule #1387

MikeStall commented Oct 16, 2017

ConnorMcMahon Oct 17, 2017 •

edited by MikeStall

Loading

ConnorMcMahon Oct 17, 2017

ConnorMcMahon Oct 17, 2017 •

edited by MikeStall

Loading

MikeStall Oct 17, 2017 •

edited

Loading

ConnorMcMahon Oct 17, 2017 •

edited by MikeStall

Loading

MikeStall Oct 17, 2017

ConnorMcMahon Oct 17, 2017 •

edited

Loading

MikeStall Oct 17, 2017

ConnorMcMahon Oct 17, 2017

MikeStall Oct 17, 2017 •

edited

Loading

ConnorMcMahon Oct 17, 2017 •

edited by MikeStall

Loading

ConnorMcMahon Oct 17, 2017 •

edited by MikeStall

Loading

ConnorMcMahon Oct 17, 2017 •

edited

Loading

MikeStall Oct 17, 2017

ConnorMcMahon Oct 17, 2017 •

edited by MikeStall

Loading

MikeStall Oct 17, 2017

MikeStall Oct 17, 2017 •

edited

Loading

mathewc Oct 18, 2017 •

edited by MikeStall

Loading

mathewc Oct 18, 2017

mathewc Oct 18, 2017 •

edited by MikeStall

Loading

mathewc Oct 18, 2017 •

edited by MikeStall

Loading

MikeStall Oct 18, 2017

mathewc Oct 18, 2017 •

edited by MikeStall

Loading

mathewc Oct 18, 2017 •

edited by MikeStall

Loading

mathewc Oct 18, 2017

mathewc Oct 18, 2017 •

edited by MikeStall

Loading

mathewc Oct 18, 2017 •

edited by MikeStall

Loading

MikeStall Oct 18, 2017

mathewc Oct 18, 2017 •

edited by MikeStall

Loading

MikeStall commented Nov 8, 2017

		}


		private static FileAccess? GetFileAccessFromAttribute(Attribute attribute)


		#endregion // #region Write overloads

		public async Task<Stream> ConvertAsync(TestStreamAttribute input, CancellationToken cancellationToken)

WIP: Bind to stream rule #1387

WIP: Bind to stream rule #1387

Conversation

MikeStall commented Oct 16, 2017

ConnorMcMahon Oct 17, 2017 • edited by MikeStall Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ConnorMcMahon Oct 17, 2017 • edited by MikeStall Loading

Choose a reason for hiding this comment

MikeStall Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

ConnorMcMahon Oct 17, 2017 • edited by MikeStall Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ConnorMcMahon Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeStall Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

ConnorMcMahon Oct 17, 2017 • edited by MikeStall Loading

Choose a reason for hiding this comment

ConnorMcMahon Oct 17, 2017 • edited by MikeStall Loading

Choose a reason for hiding this comment

ConnorMcMahon Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ConnorMcMahon Oct 17, 2017 • edited by MikeStall Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeStall Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

mathewc Oct 18, 2017 • edited by MikeStall Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc Oct 18, 2017 • edited by MikeStall Loading

Choose a reason for hiding this comment

mathewc Oct 18, 2017 • edited by MikeStall Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc Oct 18, 2017 • edited by MikeStall Loading

Choose a reason for hiding this comment

mathewc Oct 18, 2017 • edited by MikeStall Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc Oct 18, 2017 • edited by MikeStall Loading

Choose a reason for hiding this comment

mathewc Oct 18, 2017 • edited by MikeStall Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc Oct 18, 2017 • edited by MikeStall Loading

Choose a reason for hiding this comment

MikeStall commented Nov 8, 2017

ConnorMcMahon Oct 17, 2017 •

edited by MikeStall

Loading

ConnorMcMahon Oct 17, 2017 •

edited by MikeStall

Loading

MikeStall Oct 17, 2017 •

edited

Loading

ConnorMcMahon Oct 17, 2017 •

edited by MikeStall

Loading

ConnorMcMahon Oct 17, 2017 •

edited

Loading

MikeStall Oct 17, 2017 •

edited

Loading

ConnorMcMahon Oct 17, 2017 •

edited by MikeStall

Loading

ConnorMcMahon Oct 17, 2017 •

edited by MikeStall

Loading

ConnorMcMahon Oct 17, 2017 •

edited

Loading

ConnorMcMahon Oct 17, 2017 •

edited by MikeStall

Loading

MikeStall Oct 17, 2017 •

edited

Loading

mathewc Oct 18, 2017 •

edited by MikeStall

Loading

mathewc Oct 18, 2017 •

edited by MikeStall

Loading

mathewc Oct 18, 2017 •

edited by MikeStall

Loading

mathewc Oct 18, 2017 •

edited by MikeStall

Loading

mathewc Oct 18, 2017 •

edited by MikeStall

Loading

mathewc Oct 18, 2017 •

edited by MikeStall

Loading

mathewc Oct 18, 2017 •

edited by MikeStall

Loading

mathewc Oct 18, 2017 •

edited by MikeStall

Loading