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

Extend Vars support. #88

Closed
blackwinter opened this issue Dec 7, 2021 · 9 comments · Fixed by #91
Closed

Extend Vars support. #88

blackwinter opened this issue Dec 7, 2021 · 9 comments · Fixed by #91
Assignees

Comments

@blackwinter
Copy link
Member

blackwinter commented Dec 7, 2021

In order to allow all Fix expression parameters to include "compile-time" variables, I'd like to propose the following changes:

  • Introduce new (custom) Fix functions (names up for discussion):
    • put_var("varName", "value"): Define single variable (with variable substitution).
    • put_vars("varName": "value", ...): Define multiple variables (without variable substitution¹).
  • Update existing expression usages:
    • do²/if/elsif/unless: Resolve variable substitutions in parameters.

¹ No support for variable substitution in options required yet.
² Currently there's no bind that accepts any parameters.

@blackwinter blackwinter self-assigned this Dec 7, 2021
@blackwinter
Copy link
Member Author

Functional review: @TobiasNx
Code review: @fsteeg

@blackwinter
Copy link
Member Author

@TobiasNx: Is this issue "ready" for you? (cf. #86 (comment))

@TobiasNx
Copy link
Collaborator

TobiasNx commented Dec 8, 2021

* Update existing expression usages:
  
  * `do`²/`if`/`elsif`/`unless`: Resolve variable substitutions in parameters.

Is else also needed here?

I have not used variables which were provided in a morph yet so I can't judge about the function.
But how about variables that are provided from the flux to the fix?

Otherwise +1

@blackwinter
Copy link
Member Author

Is else also needed here?

Not really, no. else doesn't take any parameters.

But how about variables that are provided from the flux to the fix?

They are¹ made available to any expression that resolves variables. So hopefully all of them after this issue is fixed.

¹ Well, should be, anyway; I'm unable to locate the exact mechanism right now.

blackwinter added a commit that referenced this issue Dec 9, 2021
@blackwinter blackwinter assigned TobiasNx and unassigned blackwinter Dec 9, 2021
tibdevelopment pushed a commit to TIBHannover/oersi-etl that referenced this issue Dec 10, 2021
@TobiasNx
Copy link
Collaborator

Tried with:

https://gitlab.com/oersi/oersi-etl/-/tree/FunctionalReview/data/experimental/varTest

It does not seem to work:

$ ./gradlew --refresh-dependencies run --args '/home/tobias/git/oersi-etl/data/experimental/varTest/var.flux'

> Task :run
SLF4J: Class path contains multiple SLF4J bindings.
SLF4J: Found binding in [jar:file:/home/tobias/.gradle/caches/modules-2/files-2.1/org.slf4j/slf4j-log4j12/1.7.21/7238b064d1aba20da2ac03217d700d91e02460fa/slf4j-log4j12-1.7.21.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: Found binding in [jar:file:/home/tobias/.gradle/caches/modules-2/files-2.1/org.slf4j/slf4j-simple/1.7.21/be4b3c560a37e69b6c58278116740db28832232c/slf4j-simple-1.7.21.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation.
SLF4J: Actual binding is of type [org.slf4j.impl.Log4jLoggerFactory]
2021-12-10 12:56:38 [INFO] [ETL:127] Running /home/tobias/git/oersi-etl/data/experimental/varTest/var.flux with 5 arguments (configure output details in log4j.properties)
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.google.inject.internal.cglib.core.$ReflectUtils$2 (file:/home/tobias/.gradle/caches/modules-2/files-2.1/com.google.inject/guice/3.0/9d84f15fe35e2c716a02979fb62f50a29f38aefa/guice-3.0.jar) to method java.lang.ClassLoader.defineClass(java.lang.String,byte[],int,int,java.security.ProtectionDomain)
WARNING: Please consider reporting this to the maintainers of com.google.inject.internal.cglib.core.$ReflectUtils$2
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
2021-12-10 12:56:39 [INFO] [ETL:158] Import channel var.flux, SUCCESS: n/a, FAIL-PROCESS: n/a, FAIL-VALIDATION: n/a, FAIL-WRITE: n/a, DURATION: 00:00:01
2021-12-10 12:56:39 [ERROR] [ETL:60] 
java.lang.UnsupportedOperationException
        at java.base/java.util.Collections$UnmodifiableMap.put(Collections.java:1457)
        at org.metafacture.metafix.Metafix.putVar(Metafix.java:251)
        at org.metafacture.metafix.FixMethod$1.apply(FixMethod.java:35)
        at org.metafacture.metafix.RecordTransformer.processFunction(RecordTransformer.java:188)
        at org.metafacture.metafix.RecordTransformer.processSubexpressions(RecordTransformer.java:92)
        at org.metafacture.metafix.RecordTransformer.transform(RecordTransformer.java:63)
        at org.metafacture.metafix.Metafix.endRecord(Metafix.java:139)
        at org.metafacture.json.JsonDecoder.decode(JsonDecoder.java:194)
        at org.metafacture.json.JsonDecoder.processRecord(JsonDecoder.java:147)
        at org.metafacture.json.JsonDecoder.process(JsonDecoder.java:136)
        at org.metafacture.json.JsonDecoder.process(JsonDecoder.java:44)
        at org.metafacture.io.RecordReader.emitRecord(RecordReader.java:111)
        at org.metafacture.io.RecordReader.process(RecordReader.java:100)
        at org.metafacture.io.RecordReader.process(RecordReader.java:39)
        at org.metafacture.io.FileOpener.process(FileOpener.java:100)
        at org.metafacture.io.FileOpener.process(FileOpener.java:40)
        at org.metafacture.flux.parser.StringSender.process(StringSender.java:38)
        at org.metafacture.flux.parser.Flow.start(Flow.java:110)
        at org.metafacture.flux.parser.FluxProgramm.start(FluxProgramm.java:156)
        at org.metafacture.runner.Flux.main(Flux.java:79)
        at oersi.ETL.run(ETL.java:102)
        at oersi.ETL.main(ETL.java:56)

Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.3/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 29s
3 actionable tasks: 1 executed, 2 up-to-date

@blackwinter
Copy link
Member Author

Thanks, good catch! There can be other immutable maps for vars than Metafix.NO_VARS when passing variables from Flux. Fixed in 8bf84ac.

@TobiasNx
Copy link
Collaborator

Yes, now it seems to work. Tested it with the new updated status of branch and: https://gitlab.com/oersi/oersi-etl/-/tree/FunctionalReview/data/experimental/varTest

@TobiasNx
Copy link
Collaborator

+1

@TobiasNx TobiasNx assigned blackwinter and unassigned TobiasNx Dec 13, 2021
@blackwinter
Copy link
Member Author

Thanks!

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 a pull request may close this issue.

2 participants